From b32466be8d7fda7ea2e667da2d3921e976caaf7e Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Mon, 22 Apr 2024 15:55:55 -0700 Subject: [PATCH 1/4] 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. --- .../java/io/grpc/internal/PickFirstLeafLoadBalancer.java | 4 +--- .../io/grpc/internal/PickFirstLoadBalancerProvider.java | 6 ++++++ .../io/grpc/internal/PickFirstLeafLoadBalancerTest.java | 8 ++++---- xds/src/main/java/io/grpc/xds/XdsEndpointResource.java | 2 +- .../io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java | 4 ++-- 5 files changed, 14 insertions(+), 10 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..43a9ad28b9f 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java @@ -33,11 +33,17 @@ * 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); + public static boolean isEnabledHappyEyeballs() { + return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false); + } + @Override public boolean isAvailable() { return true; 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/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/WeightedRoundRobinLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java index 2277eb8f5e1..7f06d08998b 100644 --- a/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java @@ -56,7 +56,7 @@ import io.grpc.Status; import io.grpc.SynchronizationContext; import io.grpc.internal.FakeClock; -import io.grpc.internal.GrpcUtil; +import io.grpc.internal.PickFirstLoadBalancerProvider; import io.grpc.internal.TestUtils; import io.grpc.services.InternalCallMetricRecorder; import io.grpc.services.MetricReport; @@ -580,7 +580,7 @@ weightedChild2.new OrcaReportListener(weightedConfig.errorUtilizationPenalty).on } private boolean isEnabledHappyEyeballs() { - return GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS", true); + return PickFirstLoadBalancerProvider.isEnabledHappyEyeballs(); } @Test From 863d811c48483a08fd8fdeb0aec3f793517ac660 Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Mon, 22 Apr 2024 16:46:12 -0700 Subject: [PATCH 2/4] Set expected requestConnection count based on whether happy eyeballs is enabled or not --- .../test/java/io/grpc/xds/RingHashLoadBalancerTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java index 339779cf5e0..513724ae321 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,8 @@ 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(getSubChannel(servers.get(1)), times(expectedTimes)).requestConnection(); // kicked off connection to server2 assertThat(subchannels.size()).isEqualTo(2); // no excessive connection deliverSubchannelState(getSubChannel(servers.get(1)), CSI_CONNECTING); From 500f767ee3869630bb9012a12cb674cb34e2581f Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Tue, 7 May 2024 15:57:42 -0700 Subject: [PATCH 3/4] Disable new PickFirstLB --- .../java/io/grpc/internal/PickFirstLoadBalancerProvider.java | 2 +- xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java index 43a9ad28b9f..0301aedd73b 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java @@ -38,7 +38,7 @@ public final class PickFirstLoadBalancerProvider extends LoadBalancerProvider { 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); diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java index 513724ae321..de871cdd8f1 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java @@ -446,7 +446,8 @@ public void skipFailingHosts_pickNextNonFailingHost() { assertThat(result.getStatus().isOk()).isTrue(); assertThat(result.getSubchannel()).isNull(); // buffer request int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2; - verify(getSubChannel(servers.get(1)), times(expectedTimes)).requestConnection(); // kicked off connection to server2 + // 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); From 5713d3d9807128fb0769b1366cca18e74cc8fb29 Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Tue, 7 May 2024 16:34:12 -0700 Subject: [PATCH 4/4] Fix test expectations to handle both new and old PF LB paths. --- .../PickFirstLoadBalancerProvider.java | 5 ++++ .../java/io/grpc/rls/RlsLoadBalancerTest.java | 27 ++++++++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java index 0301aedd73b..ad33a0b8411 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java @@ -44,6 +44,11 @@ public static boolean isEnabledHappyEyeballs() { return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false); } + @VisibleForTesting + public static boolean isEnableNewPickFirst() { + return enableNewPickFirst; + } + @Override public boolean isAvailable() { return true; 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();