From 233879c82904aeda3c8018df3cdf654e60341728 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 1 Dec 2021 14:10:19 -0500 Subject: [PATCH 01/11] fix(rules): signal connections opened by rules may be immediately closed Related to #756 --- .../java/io/cryostat/rules/RuleProcessor.java | 62 ++++++++++--------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/src/main/java/io/cryostat/rules/RuleProcessor.java b/src/main/java/io/cryostat/rules/RuleProcessor.java index 25e3e1ee5a..5760b2236b 100644 --- a/src/main/java/io/cryostat/rules/RuleProcessor.java +++ b/src/main/java/io/cryostat/rules/RuleProcessor.java @@ -232,17 +232,19 @@ private void archiveRuleRecording(ConnectionDescriptor connectionDescriptor, Rul targetConnectionManager.executeConnectedTask( connectionDescriptor, connection -> { - IRecordingDescriptor descriptor = - connection.getService().getSnapshotRecording(); - try { - recordingArchiveHelper - .saveRecording(connectionDescriptor, descriptor.getName()) - .get(); - } finally { - connection.getService().close(descriptor); - } + try (connection) { + IRecordingDescriptor descriptor = + connection.getService().getSnapshotRecording(); + try { + recordingArchiveHelper + .saveRecording(connectionDescriptor, descriptor.getName()) + .get(); + } finally { + connection.getService().close(descriptor); + } - return null; + return null; + } }); } @@ -252,26 +254,28 @@ private void startRuleRecording(ConnectionDescriptor connectionDescriptor, Rule targetConnectionManager.executeConnectedTask( connectionDescriptor, connection -> { - RecordingOptionsBuilder builder = - recordingOptionsBuilderFactory - .create(connection.getService()) - .name(rule.getRecordingName()); - if (rule.getMaxAgeSeconds() > 0) { - builder = builder.maxAge(rule.getMaxAgeSeconds()).toDisk(true); - } - if (rule.getMaxSizeBytes() > 0) { - builder = builder.maxSize(rule.getMaxSizeBytes()).toDisk(true); + try (connection) { + RecordingOptionsBuilder builder = + recordingOptionsBuilderFactory + .create(connection.getService()) + .name(rule.getRecordingName()); + if (rule.getMaxAgeSeconds() > 0) { + builder = builder.maxAge(rule.getMaxAgeSeconds()).toDisk(true); + } + if (rule.getMaxSizeBytes() > 0) { + builder = builder.maxSize(rule.getMaxSizeBytes()).toDisk(true); + } + Pair template = + RecordingTargetHelper.parseEventSpecifierToTemplate( + rule.getEventSpecifier()); + recordingTargetHelper.startRecording( + true, + connectionDescriptor, + builder.build(), + template.getLeft(), + template.getRight()); + return null; } - Pair template = - RecordingTargetHelper.parseEventSpecifierToTemplate( - rule.getEventSpecifier()); - recordingTargetHelper.startRecording( - true, - connectionDescriptor, - builder.build(), - template.getLeft(), - template.getRight()); - return null; }); } } From 838cf0b900fdb9b65b34f90866385b33abc508e2 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 1 Dec 2021 14:12:02 -0500 Subject: [PATCH 02/11] fix(pom): remove itest connection cache size config Remove connection cache size/TTL config, which used an incorrect/outdated cache max size env var name, to better reflect a standard Cryostat deployment using default cache settings (unlimited size, 10s TTL) Fixes #762 --- pom.xml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pom.xml b/pom.xml index a676c7c1be..a1d89da1a7 100644 --- a/pom.xml +++ b/pom.xml @@ -396,10 +396,6 @@ --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 From 9ff6f602bdc9b0d42bb50aa25bfb1a09943c4e73 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 1 Dec 2021 14:13:33 -0500 Subject: [PATCH 03/11] fix(rules): do not create cached connections for automated rules Related to #756 Automated rules processing will reuse existing cached connections if available. If none are available then a new connection will be created and NOT cached, in order to avoid automated rules causing evictions of cached connections for other interactive clients --- .../cryostat/net/TargetConnectionManager.java | 106 +++++++++++------- .../recordings/RecordingArchiveHelper.java | 3 +- .../recordings/RecordingTargetHelper.java | 3 +- .../java/io/cryostat/rules/RuleProcessor.java | 6 +- .../RecordingArchiveHelperTest.java | 21 ++-- .../io/cryostat/rules/RuleProcessorTest.java | 8 +- 6 files changed, 94 insertions(+), 53 deletions(-) diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index a96ac0e9af..aec40f8a3b 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -55,7 +55,6 @@ import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; import com.github.benmanes.caffeine.cache.RemovalCause; -import com.github.benmanes.caffeine.cache.RemovalListener; import com.github.benmanes.caffeine.cache.Scheduler; import dagger.Lazy; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -89,42 +88,7 @@ public class TargetConnectionManager { .executor(executor) .scheduler(scheduler) .expireAfterAccess(ttl) - .removalListener( - new RemovalListener() { - @Override - public void onRemoval( - ConnectionDescriptor descriptor, - JFRConnection connection, - RemovalCause cause) { - if (descriptor == null) { - logger.warn( - "Connection eviction triggered with null descriptor"); - return; - } - if (connection == null) { - logger.warn( - "Connection eviction triggered with null connection"); - return; - } - JMXConnectionClosed evt = - new JMXConnectionClosed(descriptor.getTargetId()); - logger.info( - "Removing cached connection for {}", - descriptor.getTargetId()); - evt.begin(); - try { - connection.close(); - } catch (RuntimeException e) { - evt.setExceptionThrown(true); - throw e; - } finally { - evt.end(); - if (evt.shouldCommit()) { - evt.commit(); - } - } - } - }); + .removalListener(this::closeConnection); if (maxTargetConnections >= 0) { cacheBuilder = cacheBuilder.maximumSize(maxTargetConnections); } @@ -133,7 +97,40 @@ public void onRemoval( public T executeConnectedTask( ConnectionDescriptor connectionDescriptor, ConnectedTask task) throws Exception { - return task.execute(connections.get(connectionDescriptor)); + return executeConnectedTask(connectionDescriptor, task, true); + } + + /** + * Execute a {@link ConnectedTask}, optionally caching the connection for future re-use. If + * useCache is true then the connection will be retrieved from cache if available, or created + * and stored in the cache if not. This is subject to the cache maxSize and TTL policy. If + * useCache is false then a connection will be taken from cache if available, otherwise a new + * connection will be created externally from the cache. After the task has completed the + * connection will be closed only if the connection was not originally retrieved from the cache, + * otherwise the connection is left as-is to be subject to the cache's standard eviction policy. + * "Interactive" use cases should prefer to call this with useCache==true (or simply call {@link + * #executeConnectedTask(ConnectionDescriptor cd, ConnectedTask task)} instead). Automated use + * cases such as Automated Rules should call this with useCache==false. + */ + public T executeConnectedTask( + ConnectionDescriptor connectionDescriptor, ConnectedTask task, boolean useCache) + throws Exception { + if (useCache) { + return task.execute(connections.get(connectionDescriptor)); + } else { + JFRConnection connection = connections.getIfPresent(connectionDescriptor); + boolean cached = connection != null; + if (!cached) { + connection = connect(connectionDescriptor); + } + try { + return task.execute(connection); + } finally { + if (!cached) { + connection.close(); + } + } + } } /** @@ -153,6 +150,29 @@ public boolean markConnectionInUse(ConnectionDescriptor connectionDescriptor) { return connections.getIfPresent(connectionDescriptor) != null; } + private void closeConnection( + ConnectionDescriptor descriptor, JFRConnection connection, RemovalCause cause) { + try { + JMXConnectionClosed evt = + new JMXConnectionClosed(descriptor.getTargetId(), cause.name()); + logger.info("Removing cached connection for {}: {}", descriptor.getTargetId(), cause); + evt.begin(); + try { + connection.close(); + } catch (RuntimeException e) { + evt.setExceptionThrown(true); + throw e; + } finally { + evt.end(); + if (evt.shouldCommit()) { + evt.commit(); + } + } + } catch (Exception e) { + logger.error(e); + } + } + private JFRConnection connect(ConnectionDescriptor connectionDescriptor) throws Exception { try { return attemptConnectAsJMXServiceURL(connectionDescriptor); @@ -199,7 +219,11 @@ private JFRConnection connect( .connect( url, credentials.orElse(null), - Collections.singletonList(() -> this.connections.invalidate(cacheKey))); + Collections.singletonList( + () -> { + logger.info("Connection for {} closed", url); + this.connections.invalidate(cacheKey); + })); } catch (Exception e) { evt.setExceptionThrown(true); throw e; @@ -244,10 +268,12 @@ void setExceptionThrown(boolean exceptionThrown) { public static class JMXConnectionClosed extends Event { String serviceUri; boolean exceptionThrown; + String reason; - JMXConnectionClosed(String serviceUri) { + JMXConnectionClosed(String serviceUri, String reason) { this.serviceUri = serviceUri; this.exceptionThrown = false; + this.reason = reason; } void setExceptionThrown(boolean exceptionThrown) { diff --git a/src/main/java/io/cryostat/recordings/RecordingArchiveHelper.java b/src/main/java/io/cryostat/recordings/RecordingArchiveHelper.java index 310ea7c633..100cd53ba6 100644 --- a/src/main/java/io/cryostat/recordings/RecordingArchiveHelper.java +++ b/src/main/java/io/cryostat/recordings/RecordingArchiveHelper.java @@ -141,7 +141,8 @@ public Future saveRecording( throw new RecordingNotFoundException( "active recordings", recordingName); } - }); + }, + false); future.complete(saveName); notificationFactory .createBuilder() diff --git a/src/main/java/io/cryostat/recordings/RecordingTargetHelper.java b/src/main/java/io/cryostat/recordings/RecordingTargetHelper.java index 00dad12f7b..4f1b3424af 100644 --- a/src/main/java/io/cryostat/recordings/RecordingTargetHelper.java +++ b/src/main/java/io/cryostat/recordings/RecordingTargetHelper.java @@ -120,7 +120,8 @@ public IRecordingDescriptor startRecording( .build() .send(); return desc; - }); + }, + false); } public IRecordingDescriptor startRecording( diff --git a/src/main/java/io/cryostat/rules/RuleProcessor.java b/src/main/java/io/cryostat/rules/RuleProcessor.java index 5760b2236b..1de13bea84 100644 --- a/src/main/java/io/cryostat/rules/RuleProcessor.java +++ b/src/main/java/io/cryostat/rules/RuleProcessor.java @@ -245,7 +245,8 @@ private void archiveRuleRecording(ConnectionDescriptor connectionDescriptor, Rul return null; } - }); + }, + false); } private void startRuleRecording(ConnectionDescriptor connectionDescriptor, Rule rule) @@ -276,6 +277,7 @@ private void startRuleRecording(ConnectionDescriptor connectionDescriptor, Rule template.getRight()); return null; } - }); + }, + false); } } diff --git a/src/test/java/io/cryostat/recordings/RecordingArchiveHelperTest.java b/src/test/java/io/cryostat/recordings/RecordingArchiveHelperTest.java index 5023cf0b60..cd292947cf 100644 --- a/src/test/java/io/cryostat/recordings/RecordingArchiveHelperTest.java +++ b/src/test/java/io/cryostat/recordings/RecordingArchiveHelperTest.java @@ -144,7 +144,8 @@ void saveRecordingShouldThrowIfNoMatchingRecordingFound() throws Exception { Mockito.when( targetConnectionManager.executeConnectedTask( Mockito.any(), - Mockito.any(TargetConnectionManager.ConnectedTask.class))) + Mockito.any(TargetConnectionManager.ConnectedTask.class), + Mockito.anyBoolean())) .thenAnswer( new Answer<>() { @Override @@ -178,7 +179,8 @@ void shouldSaveRecordingWithAlias() throws Exception { Mockito.when( targetConnectionManager.executeConnectedTask( Mockito.any(), - Mockito.any(TargetConnectionManager.ConnectedTask.class))) + Mockito.any(TargetConnectionManager.ConnectedTask.class), + Mockito.anyBoolean())) .thenAnswer( new Answer<>() { @Override @@ -265,7 +267,8 @@ void shouldSaveRecordingWithNonFilesystemSafeAlias(String alias) throws Exceptio Mockito.when( targetConnectionManager.executeConnectedTask( Mockito.any(), - Mockito.any(TargetConnectionManager.ConnectedTask.class))) + Mockito.any(TargetConnectionManager.ConnectedTask.class), + Mockito.anyBoolean())) .thenAnswer( new Answer<>() { @Override @@ -337,7 +340,8 @@ void shouldSaveRecordingWithoutAlias() throws Exception { Mockito.when( targetConnectionManager.executeConnectedTask( Mockito.any(), - Mockito.any(TargetConnectionManager.ConnectedTask.class))) + Mockito.any(TargetConnectionManager.ConnectedTask.class), + Mockito.anyBoolean())) .thenAnswer( new Answer<>() { @Override @@ -420,7 +424,8 @@ void shouldSaveRecordingWithoutServiceRef() throws Exception { Mockito.when( targetConnectionManager.executeConnectedTask( Mockito.any(), - Mockito.any(TargetConnectionManager.ConnectedTask.class))) + Mockito.any(TargetConnectionManager.ConnectedTask.class), + Mockito.anyBoolean())) .thenAnswer( new Answer<>() { @Override @@ -498,7 +503,8 @@ void shouldSaveRecordingThatEndsWithJfr() throws Exception { Mockito.when( targetConnectionManager.executeConnectedTask( Mockito.any(), - Mockito.any(TargetConnectionManager.ConnectedTask.class))) + Mockito.any(TargetConnectionManager.ConnectedTask.class), + Mockito.anyBoolean())) .thenAnswer( new Answer<>() { @Override @@ -631,7 +637,8 @@ void shouldSaveRecordingNumberedCopy() throws Exception { Mockito.when( targetConnectionManager.executeConnectedTask( Mockito.any(), - Mockito.any(TargetConnectionManager.ConnectedTask.class))) + Mockito.any(TargetConnectionManager.ConnectedTask.class), + Mockito.anyBoolean())) .thenAnswer( new Answer<>() { @Override diff --git a/src/test/java/io/cryostat/rules/RuleProcessorTest.java b/src/test/java/io/cryostat/rules/RuleProcessorTest.java index 88405f120f..703015b7dc 100644 --- a/src/test/java/io/cryostat/rules/RuleProcessorTest.java +++ b/src/test/java/io/cryostat/rules/RuleProcessorTest.java @@ -150,7 +150,9 @@ void testSuccessfulRuleActivationWithCredentials() throws Exception { IConstrainedMap recordingOptions = Mockito.mock(IConstrainedMap.class); Mockito.when(recordingOptionsBuilder.build()).thenReturn(recordingOptions); - Mockito.when(targetConnectionManager.executeConnectedTask(Mockito.any(), Mockito.any())) + Mockito.when( + targetConnectionManager.executeConnectedTask( + Mockito.any(), Mockito.any(), Mockito.anyBoolean())) .thenAnswer( arg0 -> ((TargetConnectionManager.ConnectedTask) @@ -239,7 +241,9 @@ void testSuccessfulRuleActivationWithCredentials() throws Exception { @Test void testSuccessfulArchiverRuleActivationWithCredentials() throws Exception { - Mockito.when(targetConnectionManager.executeConnectedTask(Mockito.any(), Mockito.any())) + Mockito.when( + targetConnectionManager.executeConnectedTask( + Mockito.any(), Mockito.any(), Mockito.anyBoolean())) .thenAnswer( arg0 -> ((TargetConnectionManager.ConnectedTask) From a37734ef8422a6f2dbe845b7593cb086fc5c1f1c Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Thu, 2 Dec 2021 14:40:56 -0500 Subject: [PATCH 04/11] remove unnecessary toString() --- src/main/java/io/cryostat/net/TargetConnectionManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index aec40f8a3b..75c8772daf 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -211,7 +211,7 @@ private JFRConnection connect( ConnectionDescriptor cacheKey, JMXServiceURL url, Optional credentials) throws Exception { JMXConnectionOpened evt = new JMXConnectionOpened(url.toString()); - logger.info("Creating connection for {}", url.toString()); + logger.info("Creating connection for {}", url); evt.begin(); try { return jfrConnectionToolkit From 8cc3f87b6b4a02141b2513a712e363ce9b5d12cc Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Thu, 2 Dec 2021 14:43:03 -0500 Subject: [PATCH 05/11] re-add null guards --- .../java/io/cryostat/net/TargetConnectionManager.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index 75c8772daf..f4fda25934 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -152,6 +152,14 @@ public boolean markConnectionInUse(ConnectionDescriptor connectionDescriptor) { private void closeConnection( ConnectionDescriptor descriptor, JFRConnection connection, RemovalCause cause) { + if (descriptor == null) { + logger.error("Connection eviction triggered with null descriptor"); + return; + } + if (connection == null) { + logger.error("Connection eviction triggered with null connection"); + return; + } try { JMXConnectionClosed evt = new JMXConnectionClosed(descriptor.getTargetId(), cause.name()); From 6e8a8d6f4efe7d87d2fdc4c3d07a0c705de6e41c Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Thu, 2 Dec 2021 14:44:58 -0500 Subject: [PATCH 06/11] allow TargetConnectionManager to handle closing connection --- .../java/io/cryostat/rules/RuleProcessor.java | 62 +++++++++---------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/src/main/java/io/cryostat/rules/RuleProcessor.java b/src/main/java/io/cryostat/rules/RuleProcessor.java index 1de13bea84..b1aab2ee27 100644 --- a/src/main/java/io/cryostat/rules/RuleProcessor.java +++ b/src/main/java/io/cryostat/rules/RuleProcessor.java @@ -232,19 +232,17 @@ private void archiveRuleRecording(ConnectionDescriptor connectionDescriptor, Rul targetConnectionManager.executeConnectedTask( connectionDescriptor, connection -> { - try (connection) { - IRecordingDescriptor descriptor = - connection.getService().getSnapshotRecording(); - try { - recordingArchiveHelper - .saveRecording(connectionDescriptor, descriptor.getName()) - .get(); - } finally { - connection.getService().close(descriptor); - } - - return null; + IRecordingDescriptor descriptor = + connection.getService().getSnapshotRecording(); + try { + recordingArchiveHelper + .saveRecording(connectionDescriptor, descriptor.getName()) + .get(); + } finally { + connection.getService().close(descriptor); } + + return null; }, false); } @@ -255,28 +253,26 @@ private void startRuleRecording(ConnectionDescriptor connectionDescriptor, Rule targetConnectionManager.executeConnectedTask( connectionDescriptor, connection -> { - try (connection) { - RecordingOptionsBuilder builder = - recordingOptionsBuilderFactory - .create(connection.getService()) - .name(rule.getRecordingName()); - if (rule.getMaxAgeSeconds() > 0) { - builder = builder.maxAge(rule.getMaxAgeSeconds()).toDisk(true); - } - if (rule.getMaxSizeBytes() > 0) { - builder = builder.maxSize(rule.getMaxSizeBytes()).toDisk(true); - } - Pair template = - RecordingTargetHelper.parseEventSpecifierToTemplate( - rule.getEventSpecifier()); - recordingTargetHelper.startRecording( - true, - connectionDescriptor, - builder.build(), - template.getLeft(), - template.getRight()); - return null; + RecordingOptionsBuilder builder = + recordingOptionsBuilderFactory + .create(connection.getService()) + .name(rule.getRecordingName()); + if (rule.getMaxAgeSeconds() > 0) { + builder = builder.maxAge(rule.getMaxAgeSeconds()).toDisk(true); + } + if (rule.getMaxSizeBytes() > 0) { + builder = builder.maxSize(rule.getMaxSizeBytes()).toDisk(true); } + Pair template = + RecordingTargetHelper.parseEventSpecifierToTemplate( + rule.getEventSpecifier()); + recordingTargetHelper.startRecording( + true, + connectionDescriptor, + builder.build(), + template.getLeft(), + template.getRight()); + return null; }, false); } From 3d3c565cc10fc6bd570031f63b5ae7f3a74124e3 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 6 Dec 2021 10:49:48 -0500 Subject: [PATCH 07/11] fix(activereports): fix classcastexception --- .../http/api/beta/TargetReportGetHandler.java | 26 +++++++++++++------ .../http/api/v1/TargetReportGetHandler.java | 26 +++++++++++++------ 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/src/main/java/io/cryostat/net/web/http/api/beta/TargetReportGetHandler.java b/src/main/java/io/cryostat/net/web/http/api/beta/TargetReportGetHandler.java index 9209d8c5b2..a02ff48f20 100644 --- a/src/main/java/io/cryostat/net/web/http/api/beta/TargetReportGetHandler.java +++ b/src/main/java/io/cryostat/net/web/http/api/beta/TargetReportGetHandler.java @@ -127,8 +127,7 @@ public void handleWithValidJwt(RoutingContext ctx, JWT jwt) throws Exception { Exception rootCause = (Exception) ExceptionUtils.getRootCause(ee); - if (rootCause instanceof RecordingNotFoundException - || targetRecordingNotFound(rootCause)) { + if (targetRecordingNotFound(rootCause)) { throw new HttpStatusException(404, ee); } throw ee; @@ -136,11 +135,22 @@ public void handleWithValidJwt(RoutingContext ctx, JWT jwt) throws Exception { } private boolean targetRecordingNotFound(Exception rootCause) { - return rootCause instanceof SubprocessReportGenerator.ReportGenerationException - && (((SubprocessReportGenerator.ReportGenerationException) rootCause) - .getStatus() - == SubprocessReportGenerator.ExitStatus.TARGET_CONNECTION_FAILURE) - || (((SubprocessReportGenerator.ReportGenerationException) rootCause).getStatus() - == SubprocessReportGenerator.ExitStatus.NO_SUCH_RECORDING); + if (rootCause instanceof RecordingNotFoundException) { + return true; + } + boolean isReportGenerationException = + rootCause instanceof SubprocessReportGenerator.ReportGenerationException; + if (!isReportGenerationException) { + return false; + } + SubprocessReportGenerator.ReportGenerationException generationException = + (SubprocessReportGenerator.ReportGenerationException) rootCause; + boolean isTargetConnectionFailure = + generationException.getStatus() + == SubprocessReportGenerator.ExitStatus.TARGET_CONNECTION_FAILURE; + boolean isNoSuchRecording = + generationException.getStatus() + == SubprocessReportGenerator.ExitStatus.NO_SUCH_RECORDING; + return isTargetConnectionFailure || isNoSuchRecording; } } diff --git a/src/main/java/io/cryostat/net/web/http/api/v1/TargetReportGetHandler.java b/src/main/java/io/cryostat/net/web/http/api/v1/TargetReportGetHandler.java index 194330952b..0939833dd4 100644 --- a/src/main/java/io/cryostat/net/web/http/api/v1/TargetReportGetHandler.java +++ b/src/main/java/io/cryostat/net/web/http/api/v1/TargetReportGetHandler.java @@ -120,8 +120,7 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception { Exception rootCause = (Exception) ExceptionUtils.getRootCause(ee); - if (rootCause instanceof RecordingNotFoundException - || targetRecordingNotFound(rootCause)) { + if (targetRecordingNotFound(rootCause)) { throw new HttpStatusException(404, ee); } throw ee; @@ -129,11 +128,22 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception { } private boolean targetRecordingNotFound(Exception rootCause) { - return rootCause instanceof SubprocessReportGenerator.ReportGenerationException - && (((SubprocessReportGenerator.ReportGenerationException) rootCause) - .getStatus() - == SubprocessReportGenerator.ExitStatus.TARGET_CONNECTION_FAILURE) - || (((SubprocessReportGenerator.ReportGenerationException) rootCause).getStatus() - == SubprocessReportGenerator.ExitStatus.NO_SUCH_RECORDING); + if (rootCause instanceof RecordingNotFoundException) { + return true; + } + boolean isReportGenerationException = + rootCause instanceof SubprocessReportGenerator.ReportGenerationException; + if (!isReportGenerationException) { + return false; + } + SubprocessReportGenerator.ReportGenerationException generationException = + (SubprocessReportGenerator.ReportGenerationException) rootCause; + boolean isTargetConnectionFailure = + generationException.getStatus() + == SubprocessReportGenerator.ExitStatus.TARGET_CONNECTION_FAILURE; + boolean isNoSuchRecording = + generationException.getStatus() + == SubprocessReportGenerator.ExitStatus.NO_SUCH_RECORDING; + return isTargetConnectionFailure || isNoSuchRecording; } } From 1d3a330cbb2cfeb2c01e8b334ee635d33a4e0118 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 6 Dec 2021 11:37:08 -0500 Subject: [PATCH 08/11] fix(itest): correct /api/v1 to /api/beta in JWT asset download tests --- src/test/java/itest/ReportJwtDownloadIT.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/test/java/itest/ReportJwtDownloadIT.java b/src/test/java/itest/ReportJwtDownloadIT.java index f807120c02..554834bc7b 100644 --- a/src/test/java/itest/ReportJwtDownloadIT.java +++ b/src/test/java/itest/ReportJwtDownloadIT.java @@ -61,7 +61,11 @@ void testDownloadRecordingUsingJwt() throws Exception { Path assetDownload = null; try { resource = createRecording(); - String downloadUrl = getTokenDownloadUrl(new URL(resource.getString("reportUrl"))); + String downloadUrl = + getTokenDownloadUrl( + new URL( + resource.getString("reportUrl") + .replace("/api/v1/", "/api/beta/"))); Thread.sleep(10_000L); assetDownload = downloadFileAbs(downloadUrl, TEST_RECORDING_NAME, ".html") From c74d6088f72b287e8f82bcf3aa2341e31fdb93ab Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 6 Dec 2021 11:53:22 -0500 Subject: [PATCH 09/11] test(pom): set itest cache TTL to 60s --- pom.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pom.xml b/pom.xml index a1d89da1a7..852bd8fa95 100644 --- a/pom.xml +++ b/pom.xml @@ -396,6 +396,8 @@ --mount type=tmpfs,target=/opt/cryostat.d/truststore.d --env + CRYOSTAT_TARGET_CACHE_TTL=60 + --env CRYOSTAT_DISABLE_JMX_AUTH=true --env CRYOSTAT_DISABLE_SSL=true From 24aa160de1678840412e2b6e088ff44ce5fe65f5 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 6 Dec 2021 12:21:02 -0500 Subject: [PATCH 10/11] feat(tcm): listen for target LOST events and invalidate from cache --- .../java/io/cryostat/net/NetworkModule.java | 3 +++ .../cryostat/net/TargetConnectionManager.java | 21 +++++++++++++++++++ .../net/TargetConnectionManagerTest.java | 6 ++++++ 3 files changed, 30 insertions(+) diff --git a/src/main/java/io/cryostat/net/NetworkModule.java b/src/main/java/io/cryostat/net/NetworkModule.java index ddc36e335c..aeb6ab1d45 100644 --- a/src/main/java/io/cryostat/net/NetworkModule.java +++ b/src/main/java/io/cryostat/net/NetworkModule.java @@ -55,6 +55,7 @@ import io.cryostat.net.reports.ReportsModule; import io.cryostat.net.security.SecurityModule; import io.cryostat.net.web.WebModule; +import io.cryostat.platform.PlatformClient; import com.github.benmanes.caffeine.cache.Scheduler; import dagger.Binds; @@ -115,11 +116,13 @@ static Duration provideMaxTargetTTL(Environment env) { @Singleton static TargetConnectionManager provideTargetConnectionManager( Lazy connectionToolkit, + PlatformClient platformClient, @Named(TARGET_CACHE_TTL) Duration maxTargetTtl, @Named(TARGET_CACHE_SIZE) int maxTargetConnections, Logger logger) { return new TargetConnectionManager( connectionToolkit, + platformClient, ForkJoinPool.commonPool(), Scheduler.systemScheduler(), maxTargetTtl, diff --git a/src/main/java/io/cryostat/net/TargetConnectionManager.java b/src/main/java/io/cryostat/net/TargetConnectionManager.java index f4fda25934..ec94c63e44 100644 --- a/src/main/java/io/cryostat/net/TargetConnectionManager.java +++ b/src/main/java/io/cryostat/net/TargetConnectionManager.java @@ -40,6 +40,7 @@ import java.net.MalformedURLException; import java.time.Duration; import java.util.Collections; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.Executor; import java.util.regex.Matcher; @@ -51,6 +52,8 @@ import io.cryostat.core.net.Credentials; import io.cryostat.core.net.JFRConnection; import io.cryostat.core.net.JFRConnectionToolkit; +import io.cryostat.core.net.discovery.JvmDiscoveryClient.EventKind; +import io.cryostat.platform.PlatformClient; import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; @@ -75,6 +78,7 @@ public class TargetConnectionManager { TargetConnectionManager( Lazy jfrConnectionToolkit, + PlatformClient platform, Executor executor, Scheduler scheduler, Duration ttl, @@ -93,6 +97,23 @@ public class TargetConnectionManager { cacheBuilder = cacheBuilder.maximumSize(maxTargetConnections); } this.connections = cacheBuilder.build(this::connect); + + // force removal of connections from cache when we're notified about targets being lost. + // This should already be taken care of by the connection close listener, but this provides + // some additional insurance in case a target disappears and the underlying JMX network + // connection doesn't immediately report itself as closed + platform.addTargetDiscoveryListener( + tde -> { + if (EventKind.LOST.equals(tde.getEventKind())) { + for (ConnectionDescriptor cd : connections.asMap().keySet()) { + if (Objects.equals( + cd.getTargetId(), + tde.getServiceRef().getServiceUri().toString())) { + connections.invalidate(cd); + } + } + } + }); } public T executeConnectedTask( diff --git a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java index 1adbb0b401..efefa72e2b 100644 --- a/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java +++ b/src/test/java/io/cryostat/net/TargetConnectionManagerTest.java @@ -47,6 +47,7 @@ import io.cryostat.core.log.Logger; import io.cryostat.core.net.JFRConnection; import io.cryostat.core.net.JFRConnectionToolkit; +import io.cryostat.platform.PlatformClient; import com.github.benmanes.caffeine.cache.Scheduler; import org.hamcrest.MatcherAssert; @@ -67,6 +68,7 @@ class TargetConnectionManagerTest { TargetConnectionManager mgr; @Mock Logger logger; @Mock JFRConnectionToolkit jfrConnectionToolkit; + @Mock PlatformClient platformClient; Duration TTL = Duration.ofMillis(250); @BeforeEach @@ -74,6 +76,7 @@ void setup() { this.mgr = new TargetConnectionManager( () -> jfrConnectionToolkit, + platformClient, ForkJoinPool.commonPool(), Scheduler.systemScheduler(), TTL, @@ -191,6 +194,7 @@ void shouldCreateNewConnectionForAccessDelayedLongerThanTTL() throws Exception { TargetConnectionManager mgr = new TargetConnectionManager( () -> jfrConnectionToolkit, + platformClient, ForkJoinPool.commonPool(), Scheduler.systemScheduler(), Duration.ofNanos(1), @@ -231,6 +235,7 @@ void shouldCreateNewConnectionWhenMaxSizeZeroed() throws Exception { TargetConnectionManager mgr = new TargetConnectionManager( () -> jfrConnectionToolkit, + platformClient, new DirectExecutor(), Scheduler.disabledScheduler(), Duration.ofSeconds(1), @@ -270,6 +275,7 @@ void shouldCreateNewConnectionPerTarget() throws Exception { TargetConnectionManager mgr = new TargetConnectionManager( () -> jfrConnectionToolkit, + platformClient, new DirectExecutor(), Scheduler.disabledScheduler(), Duration.ofNanos(1), From 641a37b25dbaade8967c48c2faace7cc82247550 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 7 Dec 2021 13:49:35 -0500 Subject: [PATCH 11/11] update README --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d63a57330a..314701a114 100644 --- a/README.md +++ b/README.md @@ -115,8 +115,10 @@ Cryostat can be configured via the following environment variables #### Configuration for JMX Cache -* `CRYOSTAT_TARGET_CACHE_MAX_CONNECTIONS`: the maximum number of JMX connections to cache. Use `-1` for an unlimited amount -* `CRYOSTAT_TARGET_CACHE_TTL`: the time to live in seconds for cached JMX connections +* `CRYOSTAT_TARGET_CACHE_SIZE`: the maximum number of JMX connections to cache. +Use `-1` for an unlimited cache size (TTL expiration only). Defaults to `-1`. +* `CRYOSTAT_TARGET_CACHE_TTL`: the time to live (in seconds) for cached JMX +connections. Defaults to `10`. #### Configuration for Logging