From ac162624bab903078ec82333e3f5a26efe2b6031 Mon Sep 17 00:00:00 2001 From: Justin Guerra Date: Mon, 25 Mar 2024 15:16:45 -0600 Subject: [PATCH] Add config to disable close on circuit breaker (#1755) --- .../connectionpool/ConnectionPoolConfig.java | 4 ++ .../ConnectionPoolConfigImpl.java | 8 +++ .../DefaultClientChannelManager.java | 2 +- .../DefaultClientChannelManagerTest.java | 62 +++++++++++++++++++ 4 files changed, 75 insertions(+), 1 deletion(-) diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfig.java b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfig.java index ea466ab276..1192cf9d32 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfig.java +++ b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfig.java @@ -57,4 +57,8 @@ public interface ConnectionPoolConfig { boolean isSecure(); boolean useIPAddrForServer(); + + default boolean isCloseOnCircuitBreakerEnabled() { + return true; + } } diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfigImpl.java b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfigImpl.java index fc736f31fb..c3cce62180 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfigImpl.java +++ b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfigImpl.java @@ -42,6 +42,9 @@ public class ConnectionPoolConfigImpl implements ConnectionPoolConfig { public static final IClientConfigKey PER_SERVER_WATERLINE = new CommonClientConfigKey<>("PerServerWaterline", DEFAULT_PER_SERVER_WATERLINE) {}; + public static final IClientConfigKey CLOSE_ON_CIRCUIT_BREAKER = + new CommonClientConfigKey<>("CloseOnCircuitBreaker", true) {}; + private final OriginName originName; private final IClientConfig clientConfig; @@ -148,4 +151,9 @@ public boolean isSecure() { public boolean useIPAddrForServer() { return clientConfig.getPropertyAsBoolean(IClientConfigKey.Keys.UseIPAddrForServer, true); } + + @Override + public boolean isCloseOnCircuitBreakerEnabled() { + return clientConfig.getPropertyAsBoolean(CLOSE_ON_CIRCUIT_BREAKER, true); + } } diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManager.java b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManager.java index fbee8bab3f..a14026060c 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManager.java +++ b/zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManager.java @@ -235,7 +235,7 @@ public boolean release(final PooledConnection conn) { conn.getChannel().id()); } - } else if (discoveryResult.isCircuitBreakerTripped()) { + } else if (connPoolConfig.isCloseOnCircuitBreakerEnabled() && discoveryResult.isCircuitBreakerTripped()) { LOG.debug( "[{}] closing conn, server circuit breaker tripped", conn.getChannel().id()); diff --git a/zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManagerTest.java b/zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManagerTest.java index 3c0f5b1f7d..8c46149742 100644 --- a/zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManagerTest.java +++ b/zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/DefaultClientChannelManagerTest.java @@ -22,6 +22,7 @@ import com.netflix.appinfo.InstanceInfo.Builder; import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.spectator.api.DefaultRegistry; +import com.netflix.spectator.api.NoopRegistry; import com.netflix.spectator.api.Registry; import com.netflix.zuul.discovery.DiscoveryResult; import com.netflix.zuul.discovery.DynamicServerResolver; @@ -31,10 +32,12 @@ import com.netflix.zuul.passport.CurrentPassport; import io.netty.channel.DefaultEventLoop; import io.netty.channel.EventLoop; +import io.netty.channel.embedded.EmbeddedChannel; import io.netty.channel.nio.NioEventLoopGroup; import io.netty.channel.socket.nio.NioSocketChannel; import io.netty.util.concurrent.Promise; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; import java.net.InetSocketAddress; import java.net.ServerSocket; @@ -47,7 +50,11 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -217,4 +224,59 @@ void initializeAndShutdown() throws Exception { clientChannelManager.shutdown(); serverSocket.close(); } + + @Test + void closeOnCircuitBreaker() { + final OriginName originName = OriginName.fromVipAndApp("whatever", "whatever"); + DefaultClientChannelManager manager = + new DefaultClientChannelManager( + originName, + new DefaultClientConfigImpl(), + Mockito.mock(DynamicServerResolver.class), + new NoopRegistry()) { + @Override + protected void updateServerStatsOnRelease(PooledConnection conn) {} + }; + + PooledConnection connection = mock(PooledConnection.class); + DiscoveryResult discoveryResult = mock(DiscoveryResult.class); + doReturn(discoveryResult).when(connection).getServer(); + doReturn(true).when(discoveryResult).isCircuitBreakerTripped(); + doReturn(new EmbeddedChannel()).when(connection).getChannel(); + + Truth.assertThat(manager.release(connection)).isFalse(); + verify(connection).setInPool(false); + verify(connection).close(); + } + + @Test + void skipCloseOnCircuitBreaker() { + final OriginName originName = OriginName.fromVipAndApp("whatever", "whatever"); + DefaultClientConfigImpl clientConfig = new DefaultClientConfigImpl(); + DefaultClientChannelManager manager = + new DefaultClientChannelManager( + originName, clientConfig, Mockito.mock(DynamicServerResolver.class), new NoopRegistry()) { + @Override + protected void updateServerStatsOnRelease(PooledConnection conn) {} + + @Override + protected void releaseHandlers(PooledConnection conn) {} + }; + + PooledConnection connection = mock(PooledConnection.class); + DiscoveryResult discoveryResult = mock(DiscoveryResult.class); + doReturn(discoveryResult).when(connection).getServer(); + doReturn(true).when(discoveryResult).isCircuitBreakerTripped(); + doReturn(true).when(connection).isActive(); + doReturn(new EmbeddedChannel()).when(connection).getChannel(); + + IConnectionPool connectionPool = mock(IConnectionPool.class); + doReturn(true).when(connectionPool).release(connection); + manager.getPerServerPools().put(discoveryResult, connectionPool); + clientConfig.set(ConnectionPoolConfigImpl.CLOSE_ON_CIRCUIT_BREAKER, false); + + Truth.assertThat(manager.release(connection)).isTrue(); + verify(connection, never()).setInPool(anyBoolean()); + verify(connection, never()).close(); + } }