From 89e53dc875d6e6546eb38231ff35813f012f60c0 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Wed, 16 Feb 2022 21:28:11 -0800 Subject: [PATCH] xds: Do not failoverpriority when IDLE->CONNECTING --- .../io/grpc/xds/PriorityLoadBalancer.java | 25 +++-- .../io/grpc/xds/PriorityLoadBalancerTest.java | 93 +++++++++++++++++++ 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java index f29239331b..0678ffc34b 100644 --- a/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/PriorityLoadBalancer.java @@ -41,6 +41,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -66,6 +67,7 @@ final class PriorityLoadBalancer extends LoadBalancer { private List priorityNames; // Config for each priority. private Map priorityConfigs; + @Nullable private String currentPriority; private ConnectivityState currentConnectivityState; private SubchannelPicker currentPicker; @@ -113,7 +115,7 @@ final class PriorityLoadBalancer extends LoadBalancer { } } if (gotoTransientFailure) { - updateOverallState(TRANSIENT_FAILURE, new ErrorPicker(error)); + updateOverallState(null, TRANSIENT_FAILURE, new ErrorPicker(error)); } } @@ -134,14 +136,14 @@ final class PriorityLoadBalancer extends LoadBalancer { new ChildLbState(priority, priorityConfigs.get(priority).ignoreReresolution); children.put(priority, child); child.updateResolvedAddresses(); - updateOverallState(CONNECTING, BUFFER_PICKER); + updateOverallState(priority, CONNECTING, BUFFER_PICKER); return; // Give priority i time to connect. } ChildLbState child = children.get(priority); child.reactivate(); if (child.connectivityState.equals(READY) || child.connectivityState.equals(IDLE)) { logger.log(XdsLogLevel.DEBUG, "Shifted to priority {0}", priority); - updateOverallState(child.connectivityState, child.picker); + updateOverallState(priority, child.connectivityState, child.picker); for (int j = i + 1; j < priorityNames.size(); j++) { String p = priorityNames.get(j); if (children.containsKey(p)) { @@ -152,20 +154,28 @@ final class PriorityLoadBalancer extends LoadBalancer { } if (child.failOverTimer != null && child.failOverTimer.isPending()) { if (reportConnecting) { - updateOverallState(CONNECTING, BUFFER_PICKER); + updateOverallState(priority, CONNECTING, BUFFER_PICKER); } return; // Give priority i time to connect. } + if (priority.equals(currentPriority) && child.connectivityState != TRANSIENT_FAILURE) { + // If the current priority is not changed into TRANSIENT_FAILURE, keep using it. + updateOverallState(priority, child.connectivityState, child.picker); + return; + } } // TODO(zdapeng): Include error details of each priority. logger.log(XdsLogLevel.DEBUG, "All priority failed"); String lastPriority = priorityNames.get(priorityNames.size() - 1); SubchannelPicker errorPicker = children.get(lastPriority).picker; - updateOverallState(TRANSIENT_FAILURE, errorPicker); + updateOverallState(lastPriority, TRANSIENT_FAILURE, errorPicker); } - private void updateOverallState(ConnectivityState state, SubchannelPicker picker) { - if (!state.equals(currentConnectivityState) || !picker.equals(currentPicker)) { + private void updateOverallState( + @Nullable String priority, ConnectivityState state, SubchannelPicker picker) { + if (!Objects.equals(priority, currentPriority) || !state.equals(currentConnectivityState) + || !picker.equals(currentPicker)) { + currentPriority = priority; currentConnectivityState = state; currentPicker = picker; helper.updateBalancingState(state, picker); @@ -201,6 +211,7 @@ final class PriorityLoadBalancer extends LoadBalancer { picker = new ErrorPicker( Status.UNAVAILABLE.withDescription("Connection timeout for priority " + priority)); logger.log(XdsLogLevel.DEBUG, "Priority {0} failed over to next", priority); + currentPriority = null; // reset currentPriority to guarantee failover happen tryNextPriority(true); } } diff --git a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java index 3b10060dfe..9a23770512 100644 --- a/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/PriorityLoadBalancerTest.java @@ -399,6 +399,99 @@ public class PriorityLoadBalancerTest { verify(balancer3).shutdown(); } + @Test + public void idleToConnectingDoesNotTriggerFailOver() { + PriorityChildConfig priorityChildConfig0 = + new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true); + PriorityChildConfig priorityChildConfig1 = + new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true); + PriorityLbConfig priorityLbConfig = + new PriorityLbConfig( + ImmutableMap.of("p0", priorityChildConfig0, "p1", priorityChildConfig1), + ImmutableList.of("p0", "p1")); + priorityLb.handleResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of()) + .setLoadBalancingPolicyConfig(priorityLbConfig) + .build()); + assertThat(fooBalancers).hasSize(1); + assertThat(fooHelpers).hasSize(1); + Helper helper0 = Iterables.getOnlyElement(fooHelpers); + + // p0 gets IDLE. + helper0.updateBalancingState( + IDLE, + BUFFER_PICKER); + assertCurrentPickerIsBufferPicker(); + + // p0 goes to CONNECTING + helper0.updateBalancingState( + IDLE, + BUFFER_PICKER); + assertCurrentPickerIsBufferPicker(); + + // no failover happened + assertThat(fooBalancers).hasSize(1); + assertThat(fooHelpers).hasSize(1); + } + + @Test + public void readyToConnectDoesNotFailOverButUpdatesPicker() { + PriorityChildConfig priorityChildConfig0 = + new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true); + PriorityChildConfig priorityChildConfig1 = + new PriorityChildConfig(new PolicySelection(fooLbProvider, new Object()), true); + PriorityLbConfig priorityLbConfig = + new PriorityLbConfig( + ImmutableMap.of("p0", priorityChildConfig0, "p1", priorityChildConfig1), + ImmutableList.of("p0", "p1")); + priorityLb.handleResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of()) + .setLoadBalancingPolicyConfig(priorityLbConfig) + .build()); + assertThat(fooBalancers).hasSize(1); + assertThat(fooHelpers).hasSize(1); + Helper helper0 = Iterables.getOnlyElement(fooHelpers); + + // p0 gets READY. + final Subchannel subchannel0 = mock(Subchannel.class); + helper0.updateBalancingState( + READY, + new SubchannelPicker() { + @Override + public PickResult pickSubchannel(PickSubchannelArgs args) { + return PickResult.withSubchannel(subchannel0); + } + }); + assertCurrentPickerPicksSubchannel(subchannel0); + + // p0 goes to CONNECTING + helper0.updateBalancingState( + IDLE, + BUFFER_PICKER); + assertCurrentPickerIsBufferPicker(); + + // no failover happened + assertThat(fooBalancers).hasSize(1); + assertThat(fooHelpers).hasSize(1); + + // resolution update without priority change does not trigger failover + Attributes.Key fooKey = Attributes.Key.create("fooKey"); + priorityLb.handleResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of()) + .setLoadBalancingPolicyConfig(priorityLbConfig) + .setAttributes(Attributes.newBuilder().set(fooKey, "barVal").build()) + .build()); + + assertCurrentPickerIsBufferPicker(); + + // no failover happened + assertThat(fooBalancers).hasSize(1); + assertThat(fooHelpers).hasSize(1); + } + @Test public void typicalPriorityFailOverFlowWithIdleUpdate() { PriorityChildConfig priorityChildConfig0 =