diff --git a/README.md b/README.md index fe6da6ae19..95aa391019 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,13 @@ size may lead to the subprocess being forcibly killed and the parent process failing to detect the reason for the failure, leading to inaccurate failure error messages and API responses. +The environment variables `CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS` and +`CRYOSTAT_TARGET_CACHE_TTL` are used to control the target JMX connection cache +behaviour. `CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS` specifies how many +connections may be held open at once. `-1` may be used to leave the cache size +unlimited. `CRYOSTAT_TARGET_CACHE_TTL` specifies how long (in seconds) these +connections will be cached before they are closed due to inactivity. + For logging, Cryostat uses SLF4J with the java.util.logging binding. The default configuration can be overridden by mounting the desired configuration file in the container, and setting the environment variable diff --git a/pom.xml b/pom.xml index 868a9456d8..e424e74d09 100644 --- a/pom.xml +++ b/pom.xml @@ -354,6 +354,10 @@ --mount type=tmpfs,target=/opt/cryostat.d/truststore.d --env + CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS=1 + --env + CRYOSTAT_TARGET_CACHE_TTL=5 + --env CRYOSTAT_DISABLE_JMX_AUTH=true --env CRYOSTAT_DISABLE_SSL=true diff --git a/run.sh b/run.sh index 3cfa1e892a..1c4638b3cb 100755 --- a/run.sh +++ b/run.sh @@ -79,6 +79,8 @@ podman run \ -e CRYOSTAT_WEB_PORT=$CRYOSTAT_WEB_PORT \ -e CRYOSTAT_EXT_WEB_PORT=$CRYOSTAT_EXT_WEB_PORT \ -e CRYOSTAT_AUTH_MANAGER=$CRYOSTAT_AUTH_MANAGER \ + -e CRYOSTAT_TARGET_CACHE_SIZE=$CRYOSTAT_TARGET_CACHE_SIZE \ + -e CRYOSTAT_TARGET_CACHE_TTL=$CRYOSTAT_TARGET_CACHE_TTL \ -e CRYOSTAT_CONFIG_PATH="/opt/cryostat.d/conf.d" \ -e CRYOSTAT_ARCHIVE_PATH="/opt/cryostat.d/recordings.d" \ -e CRYOSTAT_TEMPLATE_PATH="/opt/cryostat.d/templates.d" \ diff --git a/src/main/java/io/cryostat/net/NetworkModule.java b/src/main/java/io/cryostat/net/NetworkModule.java index 48ed6f9db0..fa1af0aaad 100644 --- a/src/main/java/io/cryostat/net/NetworkModule.java +++ b/src/main/java/io/cryostat/net/NetworkModule.java @@ -39,7 +39,10 @@ import java.net.SocketException; import java.net.UnknownHostException; +import java.time.Duration; +import java.util.concurrent.ForkJoinPool; +import javax.inject.Named; import javax.inject.Singleton; import io.cryostat.core.log.Logger; @@ -50,6 +53,7 @@ import io.cryostat.net.reports.ReportsModule; import io.cryostat.net.web.WebModule; +import com.github.benmanes.caffeine.cache.Scheduler; import dagger.Binds; import dagger.Lazy; import dagger.Module; @@ -68,6 +72,9 @@ }) public abstract class NetworkModule { + static final String TARGET_CACHE_SIZE = "CRYOSTAT_TARGET_CACHE_SIZE"; + static final String TARGET_CACHE_TTL = "CRYOSTAT_TARGET_CACHE_TTL"; + @Provides @Singleton static HttpServer provideHttpServer( @@ -88,12 +95,32 @@ static NetworkResolver provideNetworkResolver() { return new NetworkResolver(); } + @Provides + @Named(TARGET_CACHE_SIZE) + static int provideMaxTargetConnections(Environment env) { + return Integer.parseInt(env.getEnv(TARGET_CACHE_SIZE, "-1")); + } + + @Provides + @Named(TARGET_CACHE_TTL) + static Duration provideMaxTargetTTL(Environment env) { + return Duration.ofSeconds(Integer.parseInt(env.getEnv(TARGET_CACHE_TTL, "10"))); + } + @Provides @Singleton static TargetConnectionManager provideTargetConnectionManager( - Logger logger, Lazy connectionToolkit) { + Lazy connectionToolkit, + @Named(TARGET_CACHE_TTL) Duration maxTargetTtl, + @Named(TARGET_CACHE_SIZE) int maxTargetConnections, + Logger logger) { return new TargetConnectionManager( - connectionToolkit, TargetConnectionManager.DEFAULT_TTL, logger); + connectionToolkit, + ForkJoinPool.commonPool(), + Scheduler.systemScheduler(), + maxTargetTtl, + maxTargetConnections, + logger); } @Provides diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index e4375e7170..a96ac0e9af 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -41,6 +41,7 @@ import java.time.Duration; import java.util.Collections; import java.util.Optional; +import java.util.concurrent.Executor; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -68,21 +69,25 @@ public class TargetConnectionManager { public static final Pattern HOST_PORT_PAIR_PATTERN = Pattern.compile("^([^:\\s]+)(?::(\\d{1,5}))?$"); - static final Duration DEFAULT_TTL = Duration.ofSeconds(90); - private final Lazy jfrConnectionToolkit; private final Logger logger; private final LoadingCache connections; TargetConnectionManager( - Lazy jfrConnectionToolkit, Duration ttl, Logger logger) { + Lazy jfrConnectionToolkit, + Executor executor, + Scheduler scheduler, + Duration ttl, + int maxTargetConnections, + Logger logger) { this.jfrConnectionToolkit = jfrConnectionToolkit; this.logger = logger; - this.connections = + Caffeine cacheBuilder = Caffeine.newBuilder() - .scheduler(Scheduler.systemScheduler()) + .executor(executor) + .scheduler(scheduler) .expireAfterAccess(ttl) .removalListener( new RemovalListener() { @@ -119,8 +124,11 @@ public void onRemoval( } } } - }) - .build(this::connect); + }); + if (maxTargetConnections >= 0) { + cacheBuilder = cacheBuilder.maximumSize(maxTargetConnections); + } + this.connections = cacheBuilder.build(this::connect); } public T executeConnectedTask( @@ -130,8 +138,8 @@ public T executeConnectedTask( /** * Mark a connection as still in use by the consumer. Connections expire from cache and are - * automatically closed after {@link TargetConnectionManager.DEFAULT_TTL}. For long-running - * operations which may hold the connection open and active for longer than the default TTL, + * automatically closed after {@link NetworkModule.TARGET_CACHE_TTL}. For long-running + * operations which may hold the connection open and active for longer than the configured TTL, * this method provides a way for the consumer to inform the {@link TargetConnectionManager} and * its internal cache that the connection is in fact still active and should not be * expired/closed. This will extend the lifetime of the cache entry by another TTL into the @@ -186,15 +194,12 @@ private JFRConnection connect( logger.info("Creating connection for {}", url.toString()); evt.begin(); try { - JFRConnection connection = - jfrConnectionToolkit - .get() - .connect( - url, - credentials.orElse(null), - Collections.singletonList( - () -> this.connections.invalidate(cacheKey))); - return connection; + return jfrConnectionToolkit + .get() + .connect( + url, + credentials.orElse(null), + Collections.singletonList(() -> this.connections.invalidate(cacheKey))); } catch (Exception e) { evt.setExceptionThrown(true); throw e; diff --git a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java index 6453f51356..1adbb0b401 100644 --- a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java +++ b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java @@ -39,6 +39,8 @@ import java.time.Duration; import java.util.List; +import java.util.concurrent.Executor; +import java.util.concurrent.ForkJoinPool; import javax.management.remote.JMXServiceURL; @@ -46,6 +48,7 @@ import io.cryostat.core.net.JFRConnection; import io.cryostat.core.net.JFRConnectionToolkit; +import com.github.benmanes.caffeine.cache.Scheduler; import org.hamcrest.MatcherAssert; import org.hamcrest.Matchers; import org.junit.jupiter.api.BeforeEach; @@ -68,7 +71,14 @@ class TargetConnectionManagerTest { @BeforeEach void setup() { - this.mgr = new TargetConnectionManager(() -> jfrConnectionToolkit, TTL, logger); + this.mgr = + new TargetConnectionManager( + () -> jfrConnectionToolkit, + ForkJoinPool.commonPool(), + Scheduler.systemScheduler(), + TTL, + 1, + logger); } @Test @@ -180,7 +190,12 @@ public JFRConnection answer(InvocationOnMock invocation) void shouldCreateNewConnectionForAccessDelayedLongerThanTTL() throws Exception { TargetConnectionManager mgr = new TargetConnectionManager( - () -> jfrConnectionToolkit, Duration.ofNanos(1), logger); + () -> jfrConnectionToolkit, + ForkJoinPool.commonPool(), + Scheduler.systemScheduler(), + Duration.ofNanos(1), + 1, + logger); Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) .thenAnswer( new Answer() { @@ -210,4 +225,90 @@ public JFRConnection answer(InvocationOnMock invocation) JFRConnection conn2 = mgr.executeConnectedTask(desc, a -> a); MatcherAssert.assertThat(conn1, Matchers.not(Matchers.sameInstance(conn2))); } + + @Test + void shouldCreateNewConnectionWhenMaxSizeZeroed() throws Exception { + TargetConnectionManager mgr = + new TargetConnectionManager( + () -> jfrConnectionToolkit, + new DirectExecutor(), + Scheduler.disabledScheduler(), + Duration.ofSeconds(1), + 0, + logger); + Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) + .thenAnswer( + new Answer() { + @Override + public JMXServiceURL answer(InvocationOnMock args) throws Throwable { + String host = args.getArgument(0); + int port = args.getArgument(1); + return new JMXServiceURL( + "rmi", + "", + 0, + String.format("/jndi/rmi://%s:%d/jmxrmi", host, port)); + } + }); + Mockito.when(jfrConnectionToolkit.connect(Mockito.any(), Mockito.any(), Mockito.any())) + .thenAnswer( + new Answer() { + @Override + public JFRConnection answer(InvocationOnMock invocation) + throws Throwable { + return Mockito.mock(JFRConnection.class); + } + }); + ConnectionDescriptor desc = new ConnectionDescriptor("foo"); + JFRConnection conn1 = mgr.executeConnectedTask(desc, a -> a); + JFRConnection conn2 = mgr.executeConnectedTask(desc, a -> a); + MatcherAssert.assertThat(conn1, Matchers.not(Matchers.sameInstance(conn2))); + } + + @Test + void shouldCreateNewConnectionPerTarget() throws Exception { + TargetConnectionManager mgr = + new TargetConnectionManager( + () -> jfrConnectionToolkit, + new DirectExecutor(), + Scheduler.disabledScheduler(), + Duration.ofNanos(1), + -1, + logger); + Mockito.when(jfrConnectionToolkit.createServiceURL(Mockito.anyString(), Mockito.anyInt())) + .thenAnswer( + new Answer() { + @Override + public JMXServiceURL answer(InvocationOnMock args) throws Throwable { + String host = args.getArgument(0); + int port = args.getArgument(1); + return new JMXServiceURL( + "rmi", + "", + 0, + String.format("/jndi/rmi://%s:%d/jmxrmi", host, port)); + } + }); + Mockito.when(jfrConnectionToolkit.connect(Mockito.any(), Mockito.any(), Mockito.any())) + .thenAnswer( + new Answer() { + @Override + public JFRConnection answer(InvocationOnMock invocation) + throws Throwable { + return Mockito.mock(JFRConnection.class); + } + }); + ConnectionDescriptor desc1 = new ConnectionDescriptor("foo"); + ConnectionDescriptor desc2 = new ConnectionDescriptor("bar"); + JFRConnection conn1 = mgr.executeConnectedTask(desc1, a -> a); + JFRConnection conn2 = mgr.executeConnectedTask(desc2, a -> a); + MatcherAssert.assertThat(conn1, Matchers.not(Matchers.sameInstance(conn2))); + } + + static class DirectExecutor implements Executor { + @Override + public void execute(Runnable r) { + r.run(); + } + } }