From 4e8f7df589cbe5a192e705ab372f2b27957bf662 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 14 Nov 2024 12:56:24 -0800 Subject: [PATCH] util: Remove resolvedAddresses from MultiChildLb.ChildLbState It isn't actually used by MultiChildLb, and using the health API gives us more confidence that health is properly plumbed. --- .../PickFirstLoadBalancerProvider.java | 2 +- ...PickFirstLoadBalancerProviderAccessor.java | 28 +++++++ .../io/grpc/util/MultiChildLoadBalancer.java | 13 ---- .../grpc/util/RoundRobinLoadBalancerTest.java | 67 +++++++++++++++-- .../io/grpc/xds/RingHashLoadBalancerTest.java | 75 ++++++++++++++++++- 5 files changed, 160 insertions(+), 25 deletions(-) create mode 100644 core/src/testFixtures/java/io/grpc/internal/PickFirstLoadBalancerProviderAccessor.java diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java index 92178ccae2..83b3fb7d8e 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java @@ -36,7 +36,7 @@ public final class PickFirstLoadBalancerProvider extends LoadBalancerProvider { public static final String GRPC_PF_USE_HAPPY_EYEBALLS = "GRPC_PF_USE_HAPPY_EYEBALLS"; private static final String SHUFFLE_ADDRESS_LIST_KEY = "shuffleAddressList"; - private static boolean enableNewPickFirst = + static boolean enableNewPickFirst = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", false); public static boolean isEnabledHappyEyeballs() { diff --git a/core/src/testFixtures/java/io/grpc/internal/PickFirstLoadBalancerProviderAccessor.java b/core/src/testFixtures/java/io/grpc/internal/PickFirstLoadBalancerProviderAccessor.java new file mode 100644 index 0000000000..a6e94df03c --- /dev/null +++ b/core/src/testFixtures/java/io/grpc/internal/PickFirstLoadBalancerProviderAccessor.java @@ -0,0 +1,28 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.internal; + +/** + * Accessor for PickFirstLoadBalancerProvider, allowing access only during tests. + */ +public final class PickFirstLoadBalancerProviderAccessor { + private PickFirstLoadBalancerProviderAccessor() {} + + public static void setEnableNewPickFirst(boolean enableNewPickFirst) { + PickFirstLoadBalancerProvider.enableNewPickFirst = enableNewPickFirst; + } +} diff --git a/util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java b/util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java index 36a480f63c..b51d2772d3 100644 --- a/util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java @@ -191,7 +191,6 @@ public abstract class MultiChildLoadBalancer extends LoadBalancer { } newChildLbStates.add(childLbState); if (entry.getValue() != null) { - childLbState.setResolvedAddresses(entry.getValue()); // update child childLbState.lb.handleResolvedAddresses(entry.getValue()); // update child LB } } @@ -262,8 +261,6 @@ public abstract class MultiChildLoadBalancer extends LoadBalancer { */ public class ChildLbState { private final Object key; - private ResolvedAddresses resolvedAddresses; - private final LoadBalancer lb; private ConnectivityState currentState; private SubchannelPicker currentPicker = new FixedResultPicker(PickResult.withNoResult()); @@ -321,16 +318,6 @@ public abstract class MultiChildLoadBalancer extends LoadBalancer { currentPicker = newPicker; } - protected final void setResolvedAddresses(ResolvedAddresses newAddresses) { - checkNotNull(newAddresses, "Missing address list for child"); - resolvedAddresses = newAddresses; - } - - @VisibleForTesting - public final ResolvedAddresses getResolvedAddresses() { - return resolvedAddresses; - } - /** * ChildLbStateHelper is the glue between ChildLbState and the helpers associated with the * petiole policy above and the PickFirstLoadBalancer's helper below. diff --git a/util/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java b/util/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java index 0201fb578e..18854ca1bb 100644 --- a/util/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java @@ -22,7 +22,6 @@ import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.READY; import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; -import static io.grpc.util.MultiChildLoadBalancer.IS_PETIOLE_POLICY; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; @@ -54,13 +53,15 @@ import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelPicker; import io.grpc.Status; +import io.grpc.internal.PickFirstLoadBalancerProvider; +import io.grpc.internal.PickFirstLoadBalancerProviderAccessor; import io.grpc.internal.TestUtils; -import io.grpc.util.MultiChildLoadBalancer.ChildLbState; import io.grpc.util.RoundRobinLoadBalancer.ReadyPicker; import java.net.SocketAddress; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -104,6 +105,7 @@ public class RoundRobinLoadBalancerTest { private ArgumentCaptor createArgsCaptor; private TestHelper testHelperInst = new TestHelper(); private Helper mockHelper = mock(Helper.class, delegatesTo(testHelperInst)); + private boolean defaultNewPickFirst = PickFirstLoadBalancerProvider.isEnabledNewPickFirst(); @Mock // This LoadBalancer doesn't use any of the arg fields, as verified in tearDown(). private PickSubchannelArgs mockArgs; @@ -126,6 +128,7 @@ public class RoundRobinLoadBalancerTest { @After public void tearDown() throws Exception { + PickFirstLoadBalancerProviderAccessor.setEnableNewPickFirst(defaultNewPickFirst); verifyNoMoreInteractions(mockArgs); } @@ -201,12 +204,6 @@ public class RoundRobinLoadBalancerTest { verify(removedSubchannel, times(1)).requestConnection(); verify(oldSubchannel, times(1)).requestConnection(); - assertThat(loadBalancer.getChildLbStates().size()).isEqualTo(2); - for (ChildLbState childLbState : loadBalancer.getChildLbStates()) { - assertThat(childLbState.getResolvedAddresses().getAttributes().get(IS_PETIOLE_POLICY)) - .isTrue(); - } - // This time with Attributes List latestServers = Lists.newArrayList(oldEag2, newEag); @@ -464,6 +461,60 @@ public class RoundRobinLoadBalancerTest { assertThat(pickers.hasNext()).isFalse(); } + @Test + public void subchannelHealthObserved() throws Exception { + // Only the new PF policy observes the new separate listener for health + PickFirstLoadBalancerProviderAccessor.setEnableNewPickFirst(true); + // PickFirst does most of this work. If the test fails, check IS_PETIOLE_POLICY + Map healthListeners = new HashMap<>(); + loadBalancer = new RoundRobinLoadBalancer(new ForwardingLoadBalancerHelper() { + @Override + public Subchannel createSubchannel(CreateSubchannelArgs args) { + Subchannel subchannel = super.createSubchannel(args.toBuilder() + .setAttributes(args.getAttributes().toBuilder() + .set(LoadBalancer.HAS_HEALTH_PRODUCER_LISTENER_KEY, true) + .build()) + .build()); + healthListeners.put( + subchannel, args.getOption(LoadBalancer.HEALTH_CONSUMER_LISTENER_ARG_KEY)); + return subchannel; + } + + @Override + protected Helper delegate() { + return mockHelper; + } + }); + + InOrder inOrder = inOrder(mockHelper); + Status addressesAcceptanceStatus = acceptAddresses(servers, Attributes.EMPTY); + assertThat(addressesAcceptanceStatus.isOk()).isTrue(); + Subchannel subchannel0 = subchannels.get(Arrays.asList(servers.get(0))); + Subchannel subchannel1 = subchannels.get(Arrays.asList(servers.get(1))); + Subchannel subchannel2 = subchannels.get(Arrays.asList(servers.get(2))); + + // Subchannels go READY, but the LB waits for health + for (Subchannel subchannel : subchannels.values()) { + deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + } + inOrder.verify(mockHelper, times(0)) + .updateBalancingState(eq(READY), any(SubchannelPicker.class)); + + // Health results lets subchannels go READY + healthListeners.get(subchannel0).onSubchannelState( + ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE.withDescription("oh no"))); + healthListeners.get(subchannel1).onSubchannelState(ConnectivityStateInfo.forNonError(READY)); + healthListeners.get(subchannel2).onSubchannelState(ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(mockHelper, times(2)).updateBalancingState(eq(READY), pickerCaptor.capture()); + SubchannelPicker picker = pickerCaptor.getValue(); + List picks = Arrays.asList( + picker.pickSubchannel(mockArgs).getSubchannel(), + picker.pickSubchannel(mockArgs).getSubchannel(), + picker.pickSubchannel(mockArgs).getSubchannel(), + picker.pickSubchannel(mockArgs).getSubchannel()); + assertThat(picks).containsExactly(subchannel1, subchannel2, subchannel1, subchannel2); + } + @Test public void readyPicker_emptyList() { // ready picker list must be non-empty diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java index 4afe440c90..50c2ef1d54 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java @@ -23,7 +23,6 @@ import static io.grpc.ConnectivityState.IDLE; import static io.grpc.ConnectivityState.READY; import static io.grpc.ConnectivityState.SHUTDOWN; import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; -import static io.grpc.util.MultiChildLoadBalancer.IS_PETIOLE_POLICY; import static io.grpc.xds.RingHashLoadBalancerTest.InitializationFlags.DO_NOT_RESET_HELPER; import static io.grpc.xds.RingHashLoadBalancerTest.InitializationFlags.DO_NOT_VERIFY; import static io.grpc.xds.RingHashLoadBalancerTest.InitializationFlags.RESET_SUBCHANNEL_MOCKS; @@ -48,6 +47,7 @@ import io.grpc.CallOptions; import io.grpc.ConnectivityState; import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; +import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickDetailsConsumer; @@ -62,9 +62,11 @@ import io.grpc.Status.Code; import io.grpc.SynchronizationContext; import io.grpc.internal.FakeClock; import io.grpc.internal.PickFirstLoadBalancerProvider; +import io.grpc.internal.PickFirstLoadBalancerProviderAccessor; import io.grpc.internal.PickSubchannelArgsImpl; import io.grpc.testing.TestMethodDescriptors; import io.grpc.util.AbstractTestHelper; +import io.grpc.util.ForwardingLoadBalancerHelper; import io.grpc.util.MultiChildLoadBalancer.ChildLbState; import io.grpc.xds.RingHashLoadBalancer.RingHashConfig; import java.lang.Thread.UncaughtExceptionHandler; @@ -74,8 +76,11 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Deque; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Random; +import java.util.Set; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -115,6 +120,7 @@ public class RingHashLoadBalancerTest { @Captor private ArgumentCaptor pickerCaptor; private RingHashLoadBalancer loadBalancer; + private boolean defaultNewPickFirst = PickFirstLoadBalancerProvider.isEnabledNewPickFirst(); @Before public void setUp() { @@ -126,6 +132,7 @@ public class RingHashLoadBalancerTest { @After public void tearDown() { + PickFirstLoadBalancerProviderAccessor.setEnableNewPickFirst(defaultNewPickFirst); loadBalancer.shutdown(); for (Subchannel subchannel : subchannels.values()) { verify(subchannel).shutdown(); @@ -906,6 +913,70 @@ public class RingHashLoadBalancerTest { assertThat(description).contains("Address: FakeSocketAddress-server2, count: 3"); } + @Test + public void subchannelHealthObserved() throws Exception { + // Only the new PF policy observes the new separate listener for health + PickFirstLoadBalancerProviderAccessor.setEnableNewPickFirst(true); + // PickFirst does most of this work. If the test fails, check IS_PETIOLE_POLICY + Map healthListeners = new HashMap<>(); + loadBalancer = new RingHashLoadBalancer(new ForwardingLoadBalancerHelper() { + @Override + public Subchannel createSubchannel(CreateSubchannelArgs args) { + Subchannel subchannel = super.createSubchannel(args.toBuilder() + .setAttributes(args.getAttributes().toBuilder() + .set(LoadBalancer.HAS_HEALTH_PRODUCER_LISTENER_KEY, true) + .build()) + .build()); + healthListeners.put( + subchannel, args.getOption(LoadBalancer.HEALTH_CONSUMER_LISTENER_ARG_KEY)); + return subchannel; + } + + @Override + protected Helper delegate() { + return helper; + } + }); + + InOrder inOrder = Mockito.inOrder(helper); + List servers = createWeightedServerAddrs(1, 1); + initializeLbSubchannels(new RingHashConfig(10, 100), servers); + Subchannel subchannel0 = subchannels.get(Collections.singletonList(servers.get(0))); + Subchannel subchannel1 = subchannels.get(Collections.singletonList(servers.get(1))); + + // Subchannels go READY, but the LB waits for health + for (Subchannel subchannel : subchannels.values()) { + deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY)); + } + inOrder.verify(helper, times(0)).updateBalancingState(eq(READY), any(SubchannelPicker.class)); + + // Health results lets subchannels go READY + healthListeners.get(subchannel0).onSubchannelState(ConnectivityStateInfo.forNonError(READY)); + healthListeners.get(subchannel1).onSubchannelState(ConnectivityStateInfo.forNonError(READY)); + inOrder.verify(helper, times(2)).updateBalancingState(eq(READY), pickerCaptor.capture()); + SubchannelPicker picker = pickerCaptor.getValue(); + Random random = new Random(1); + Set picks = new HashSet<>(); + for (int i = 0; i < 10; i++) { + picks.add( + picker.pickSubchannel(getDefaultPickSubchannelArgs(random.nextLong())).getSubchannel()); + } + assertThat(picks).containsExactly(subchannel0, subchannel1); + + // Unhealthy subchannel skipped + healthListeners.get(subchannel0).onSubchannelState( + ConnectivityStateInfo.forTransientFailure(Status.UNAVAILABLE.withDescription("oh no"))); + inOrder.verify(helper).updateBalancingState(eq(READY), pickerCaptor.capture()); + picker = pickerCaptor.getValue(); + random.setSeed(1); + picks.clear(); + for (int i = 0; i < 10; i++) { + picks.add( + picker.pickSubchannel(getDefaultPickSubchannelArgs(random.nextLong())).getSubchannel()); + } + assertThat(picks).containsExactly(subchannel1); + } + private List initializeLbSubchannels(RingHashConfig config, List servers, InitializationFlags... initFlags) { @@ -950,8 +1021,6 @@ public class RingHashLoadBalancerTest { for (ChildLbState childLbState : loadBalancer.getChildLbStates()) { childLbState.getCurrentPicker() .pickSubchannel(getDefaultPickSubchannelArgs(hashFunc.hashVoid())); - assertThat(childLbState.getResolvedAddresses().getAttributes().get(IS_PETIOLE_POLICY)) - .isTrue(); } if (doVerifies) {