From 59b189bf914fb89989a6b773fdbc8e7b500f5207 Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Wed, 8 May 2024 10:08:23 -0700 Subject: [PATCH] Change HappyEyeballs and new pick first LB flags default value to false (#11120) * Change HappyEyeballs flag default value to false since some G3 users are seeing problems. Put the flag logic in a common place for PickFirstLeafLoadBalancer & WRR's test. * Set expected requestConnection count based on whether happy eyeballs is enabled or not * Disable new PickFirstLB * Fix test expectations to handle both new and old PF LB paths. --- .../internal/PickFirstLeafLoadBalancer.java | 4 +-- .../PickFirstLoadBalancerProvider.java | 13 ++++++++- .../PickFirstLeafLoadBalancerTest.java | 8 +++--- .../java/io/grpc/rls/RlsLoadBalancerTest.java | 27 ++++++++++++++----- .../java/io/grpc/xds/XdsEndpointResource.java | 2 +- .../io/grpc/xds/RingHashLoadBalancerTest.java | 10 ++++--- .../WeightedRoundRobinLoadBalancerTest.java | 4 +-- 7 files changed, 47 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java b/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java index bcab9991f7f..3846656355c 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java @@ -59,8 +59,6 @@ final class PickFirstLeafLoadBalancer extends LoadBalancer { private static final Logger log = Logger.getLogger(PickFirstLeafLoadBalancer.class.getName()); @VisibleForTesting static final int CONNECTION_DELAY_INTERVAL_MS = 250; - public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS = - "GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS"; private final Helper helper; private final Map subchannels = new HashMap<>(); private Index addressIndex; @@ -71,7 +69,7 @@ final class PickFirstLeafLoadBalancer extends LoadBalancer { private ConnectivityState rawConnectivityState = IDLE; private ConnectivityState concludedState = IDLE; private final boolean enableHappyEyeballs = - GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true); + PickFirstLoadBalancerProvider.isEnabledHappyEyeballs(); PickFirstLeafLoadBalancer(Helper helper) { this.helper = checkNotNull(helper, "helper"); diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java index dd862fe704b..ad33a0b8411 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java @@ -33,10 +33,21 @@ * down the address list and sticks to the first that works. */ public final class PickFirstLoadBalancerProvider extends LoadBalancerProvider { + public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS = + "GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS"; private static final String SHUFFLE_ADDRESS_LIST_KEY = "shuffleAddressList"; static boolean enableNewPickFirst = - GrpcUtil.getFlag("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", true); + GrpcUtil.getFlag("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", false); + + public static boolean isEnabledHappyEyeballs() { + return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false); + } + + @VisibleForTesting + public static boolean isEnableNewPickFirst() { + return enableNewPickFirst; + } @Override public boolean isAvailable() { diff --git a/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java b/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java index 92222ac9af6..eceb9500ef0 100644 --- a/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java @@ -142,8 +142,8 @@ public void uncaughtException(Thread t, Throwable e) { @Before public void setUp() { originalHappyEyeballsEnabledValue = - System.getProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS); - System.setProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, + System.getProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS); + System.setProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, enableHappyEyeballs ? "true" : "false"); for (int i = 1; i <= 5; i++) { @@ -173,9 +173,9 @@ public void setUp() { @After public void tearDown() { if (originalHappyEyeballsEnabledValue == null) { - System.clearProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS); + System.clearProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS); } else { - System.setProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, + System.setProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, originalHappyEyeballsEnabledValue); } diff --git a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java index a6612137b20..56cd3cd9449 100644 --- a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java @@ -70,6 +70,7 @@ import io.grpc.inprocess.InProcessServerBuilder; import io.grpc.internal.FakeClock; import io.grpc.internal.JsonParser; +import io.grpc.internal.PickFirstLoadBalancerProvider; import io.grpc.internal.PickSubchannelArgsImpl; import io.grpc.internal.testing.StreamRecorder; import io.grpc.lookup.v1.RouteLookupServiceGrpc; @@ -212,7 +213,8 @@ public void lb_serverStatusCodeConversion() throws Exception { subchannel.updateState(ConnectivityStateInfo.forNonError(ConnectivityState.READY)); res = picker.pickSubchannel(fakeSearchMethodArgs); assertThat(res.getStatus().getCode()).isEqualTo(Status.Code.OK); - verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete"); + int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2; + verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete"); // Check on conversion Throwable cause = new Throwable("cause"); @@ -244,6 +246,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception { .updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class)); inOrder.verify(helper, atLeast(0)).getSynchronizationContext(); inOrder.verify(helper, atLeast(0)).getScheduledExecutorService(); + inOrder.verify(helper, atLeast(0)).getMetricRecorder(); + inOrder.verify(helper, atLeast(0)).getChannelTarget(); inOrder.verifyNoMoreInteractions(); assertThat(res.getStatus().isOk()).isTrue(); @@ -258,7 +262,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception { res = picker.pickSubchannel(searchSubchannelArgs); assertThat(subchannelIsReady(res.getSubchannel())).isTrue(); assertThat(res.getSubchannel()).isSameInstanceAs(searchSubchannel); - verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete"); + int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2; + verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete"); // rescue should be pending status although the overall channel state is READY res = picker.pickSubchannel(rescueSubchannelArgs); @@ -367,18 +372,22 @@ public void lb_working_withDefaultTarget_noRlsResponse() throws Exception { inOrder.verify(helper).getMetricRecorder(); inOrder.verify(helper).getChannelTarget(); inOrder.verifyNoMoreInteractions(); - verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 1, 1, "defaultTarget", "complete"); + int times = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2; + verifyLongCounterAdd("grpc.lb.rls.default_target_picks", times, 1, + "defaultTarget", "complete"); verifyNoMoreInteractions(mockMetricRecorder); Subchannel subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel(); assertThat(subchannelIsReady(subchannel)).isTrue(); assertThat(subchannel).isSameInstanceAs(fallbackSubchannel); - verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 2, 1, "defaultTarget", "complete"); + verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget", + "complete"); subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel(); assertThat(subchannelIsReady(subchannel)).isTrue(); assertThat(subchannel).isSameInstanceAs(fallbackSubchannel); - verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 3, 1, "defaultTarget", "complete"); + verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget", + "complete"); // Make sure that when RLS starts communicating that default stops being used fakeThrottler.nextResult = false; @@ -389,7 +398,8 @@ public void lb_working_withDefaultTarget_noRlsResponse() throws Exception { (FakeSubchannel) markReadyAndGetPickResult(inOrder, searchSubchannelArgs).getSubchannel(); assertThat(searchSubchannel).isNotNull(); assertThat(searchSubchannel).isNotSameInstanceAs(fallbackSubchannel); - verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete"); + times = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2; + verifyLongCounterAdd("grpc.lb.rls.target_picks", times, 1, "wilderness", "complete"); // create rescue subchannel picker.pickSubchannel(rescueSubchannelArgs); @@ -433,6 +443,8 @@ public void lb_working_withoutDefaultTarget() throws Exception { .updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class)); inOrder.verify(helper, atLeast(0)).getSynchronizationContext(); inOrder.verify(helper, atLeast(0)).getScheduledExecutorService(); + inOrder.verify(helper, atLeast(0)).getMetricRecorder(); + inOrder.verify(helper, atLeast(0)).getChannelTarget(); inOrder.verifyNoMoreInteractions(); assertThat(res.getStatus().isOk()).isTrue(); @@ -469,7 +481,8 @@ public void lb_working_withoutDefaultTarget() throws Exception { res = picker.pickSubchannel(newPickSubchannelArgs(fakeSearchMethod)); assertThat(res.getStatus().isOk()).isFalse(); assertThat(subchannelIsReady(res.getSubchannel())).isFalse(); - verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete"); + int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2; + verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete"); res = picker.pickSubchannel(newPickSubchannelArgs(fakeRescueMethod)); assertThat(subchannelIsReady(res.getSubchannel())).isTrue(); diff --git a/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java b/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java index 770217bfbaa..0c7e8f46bcc 100644 --- a/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java @@ -101,7 +101,7 @@ protected EdsUpdate doParse(Args args, Message unpackedMessage) throws ResourceI } private static boolean isEnabledXdsDualStack() { - return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true); + return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false); } private static EdsUpdate processClusterLoadAssignment(ClusterLoadAssignment assignment) diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java index 339779cf5e0..de871cdd8f1 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java @@ -151,7 +151,8 @@ public void subchannelLazyConnectUntilPicked() { assertThat(result.getStatus().isOk()).isTrue(); assertThat(result.getSubchannel()).isNull(); Subchannel subchannel = Iterables.getOnlyElement(subchannels.values()); - verify(subchannel).requestConnection(); + int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2; + verify(subchannel, times(expectedTimes)).requestConnection(); verify(helper).updateBalancingState(eq(CONNECTING), any(SubchannelPicker.class)); verify(helper).createSubchannel(any(CreateSubchannelArgs.class)); deliverSubchannelState(subchannel, CSI_CONNECTING); @@ -185,7 +186,8 @@ public void subchannelNotAutoReconnectAfterReenteringIdle() { pickerCaptor.getValue().pickSubchannel(args); Subchannel subchannel = subchannels.get(Collections.singletonList(childLbState.getEag())); InOrder inOrder = Mockito.inOrder(helper, subchannel); - inOrder.verify(subchannel).requestConnection(); + int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2; + inOrder.verify(subchannel, times(expectedTimes)).requestConnection(); deliverSubchannelState(subchannel, CSI_READY); inOrder.verify(helper).updateBalancingState(eq(READY), any(SubchannelPicker.class)); deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE)); @@ -443,7 +445,9 @@ public void skipFailingHosts_pickNextNonFailingHost() { PickResult result = pickerCaptor.getValue().pickSubchannel(args); assertThat(result.getStatus().isOk()).isTrue(); assertThat(result.getSubchannel()).isNull(); // buffer request - verify(getSubChannel(servers.get(1))).requestConnection(); // kicked off connection to server2 + int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2; + // verify kicked off connection to server2 + verify(getSubChannel(servers.get(1)), times(expectedTimes)).requestConnection(); assertThat(subchannels.size()).isEqualTo(2); // no excessive connection deliverSubchannelState(getSubChannel(servers.get(1)), CSI_CONNECTING); diff --git a/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java index cf04b3b75e0..618742c41ee 100644 --- a/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java @@ -64,7 +64,7 @@ import io.grpc.inprocess.InProcessChannelBuilder; import io.grpc.inprocess.InProcessServerBuilder; import io.grpc.internal.FakeClock; -import io.grpc.internal.GrpcUtil; +import io.grpc.internal.PickFirstLoadBalancerProvider; import io.grpc.internal.TestUtils; import io.grpc.internal.testing.StreamRecorder; import io.grpc.services.InternalCallMetricRecorder; @@ -595,7 +595,7 @@ weightedChild2.new OrcaReportListener(weightedConfig.errorUtilizationPenalty).on } private boolean isEnabledHappyEyeballs() { - return GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS", true); + return PickFirstLoadBalancerProvider.isEnabledHappyEyeballs(); } @Test