From e9e3b112e37c44a9140b858cfd5c20fe5b13b7e9 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 5 Oct 2022 11:29:02 -0400 Subject: [PATCH] use RecordingTargetHelper to clean up --- .../web/http/api/v2/RuleDeleteHandler.java | 69 +++------- .../http/api/v2/RuleDeleteHandlerTest.java | 120 ++++-------------- 2 files changed, 49 insertions(+), 140 deletions(-) diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandler.java b/src/main/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandler.java index 23c21e8451..2b0f9d811e 100644 --- a/src/main/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandler.java +++ b/src/main/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandler.java @@ -50,11 +50,11 @@ import io.cryostat.messaging.notifications.NotificationFactory; import io.cryostat.net.AuthManager; import io.cryostat.net.ConnectionDescriptor; -import io.cryostat.net.TargetConnectionManager; import io.cryostat.net.security.ResourceAction; import io.cryostat.net.web.http.HttpMimeType; import io.cryostat.net.web.http.api.ApiVersion; import io.cryostat.platform.ServiceRef; +import io.cryostat.recordings.RecordingTargetHelper; import io.cryostat.rules.Rule; import io.cryostat.rules.RuleRegistry; @@ -70,7 +70,7 @@ class RuleDeleteHandler extends AbstractV2RequestHandler { private final Vertx vertx; private final RuleRegistry ruleRegistry; - private final TargetConnectionManager targetConnectionManager; + private final RecordingTargetHelper recordings; private final DiscoveryStorage storage; private final CredentialsManager credentialsManager; private final NotificationFactory notificationFactory; @@ -81,7 +81,7 @@ class RuleDeleteHandler extends AbstractV2RequestHandler { Vertx vertx, AuthManager auth, RuleRegistry ruleRegistry, - TargetConnectionManager targetConnectionManager, + RecordingTargetHelper recordings, DiscoveryStorage storage, CredentialsManager credentialsManager, NotificationFactory notificationFactory, @@ -90,7 +90,7 @@ class RuleDeleteHandler extends AbstractV2RequestHandler { super(auth, gson); this.vertx = vertx; this.ruleRegistry = ruleRegistry; - this.targetConnectionManager = targetConnectionManager; + this.recordings = recordings; this.storage = storage; this.credentialsManager = credentialsManager; this.notificationFactory = notificationFactory; @@ -147,61 +147,34 @@ public IntermediateResponse handle(RequestParameters params) throws ApiExc .build() .send(); if (Boolean.valueOf(params.getQueryParams().get(CLEAN_PARAM))) { - vertx.executeBlocking(promise -> { - cleanup(rule); - promise.complete(); - }); + vertx.executeBlocking( + promise -> { + try { + cleanup(rule); + promise.complete(); + } catch (Exception e) { + promise.fail(e); + } + }); } return new IntermediateResponse().body(null); } private void cleanup(Rule rule) { for (ServiceRef ref : storage.listDiscoverableServices()) { - vertx.executeBlocking( + vertx.executeBlocking( promise -> { try { - promise.complete(ruleRegistry.applies(rule, ref)); - if (!ruleRegistry.applies(rule, ref)) { - promise.complete(null); + if (ruleRegistry.applies(rule, ref)) { + ConnectionDescriptor cd = + new ConnectionDescriptor( + ref, credentialsManager.getCredentials(ref)); + recordings.stopRecording(cd, rule.getRecordingName()); } - } catch (Exception e) { - promise.fail(e); - } - }, - false, - result -> { - if (result.failed()) { - logger.error(new RuntimeException(result.cause())); - return; - } - if (!result.result()) { - return; - } - try { - targetConnectionManager.executeConnectedTaskAsync( - new ConnectionDescriptor( - ref, credentialsManager.getCredentials(ref)), - conn -> { - conn.getService().getAvailableRecordings().stream() - .filter( - rec -> - rec.getName() - .equals( - rule - .getRecordingName())) - .findFirst() - .ifPresent( - r -> { - try { - conn.getService().stop(r); - } catch (Exception e) { - logger.error(e); - } - }); - return null; - }); + promise.complete(); } catch (Exception e) { logger.error(e); + promise.fail(e); } }); } diff --git a/src/test/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandlerTest.java b/src/test/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandlerTest.java index 656e1fbadb..40f01e1dc1 100644 --- a/src/test/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandlerTest.java +++ b/src/test/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandlerTest.java @@ -43,24 +43,20 @@ import java.util.Optional; import java.util.Set; -import org.openjdk.jmc.rjmx.services.jfr.IFlightRecorderService; -import org.openjdk.jmc.rjmx.services.jfr.IRecordingDescriptor; - import io.cryostat.MainModule; import io.cryostat.MockVertx; import io.cryostat.configuration.CredentialsManager; import io.cryostat.core.FlightRecorderException; import io.cryostat.core.log.Logger; -import io.cryostat.core.net.JFRConnection; import io.cryostat.discovery.DiscoveryStorage; import io.cryostat.messaging.notifications.Notification; import io.cryostat.messaging.notifications.NotificationFactory; import io.cryostat.net.AuthManager; -import io.cryostat.net.TargetConnectionManager; import io.cryostat.net.security.ResourceAction; import io.cryostat.net.web.http.HttpMimeType; import io.cryostat.net.web.http.api.ApiVersion; import io.cryostat.platform.ServiceRef; +import io.cryostat.recordings.RecordingTargetHelper; import io.cryostat.rules.Rule; import io.cryostat.rules.RuleRegistry; @@ -86,7 +82,7 @@ class RuleDeleteHandlerTest { Vertx vertx = MockVertx.vertx(); @Mock AuthManager auth; @Mock RuleRegistry registry; - @Mock TargetConnectionManager targetConnectionManager; + @Mock RecordingTargetHelper recordingTargetHelper; @Mock DiscoveryStorage storage; @Mock CredentialsManager credentialsManager; @Mock NotificationFactory notificationFactory; @@ -116,7 +112,7 @@ void setup() { vertx, auth, registry, - targetConnectionManager, + recordingTargetHelper, storage, credentialsManager, notificationFactory, @@ -206,6 +202,13 @@ void shouldRespondWith404ForNonexistentRule() throws Exception { ApiException ex = Assertions.assertThrows(ApiException.class, () -> handler.handle(params)); MatcherAssert.assertThat(ex.getStatusCode(), Matchers.equalTo(404)); + + Mockito.verify(vertx, Mockito.never()).executeBlocking(Mockito.any()); + Mockito.verify(registry, Mockito.never()).deleteRule(Mockito.any(Rule.class)); + Mockito.verify(registry, Mockito.never()).deleteRule(Mockito.anyString()); + Mockito.verify(registry, Mockito.never()).applies(Mockito.any(), Mockito.any()); + Mockito.verify(recordingTargetHelper, Mockito.never()) + .stopRecording(Mockito.any(), Mockito.any()); } @Test @@ -233,12 +236,17 @@ void shouldRespondWith200ForCleanupFailures() throws Exception { FlightRecorderException exception = new FlightRecorderException(new Exception("test message")); - Mockito.when(targetConnectionManager.executeConnectedTask(Mockito.any(), Mockito.any())) + Mockito.when(recordingTargetHelper.stopRecording(Mockito.any(), Mockito.any())) .thenThrow(exception); IntermediateResponse response = handler.handle(params); MatcherAssert.assertThat(response.getStatusCode(), Matchers.equalTo(200)); + Mockito.verify(vertx, Mockito.times(2)).executeBlocking(Mockito.any()); + Mockito.verify(registry).deleteRule(rule); + Mockito.verify(registry).applies(rule, serviceRef); + Mockito.verify(recordingTargetHelper) + .stopRecording(Mockito.any(), Mockito.eq(rule.getRecordingName())); Mockito.verify(logger).error(exception); } @@ -257,78 +265,21 @@ void shouldRespondWith200AfterCleanupNoop() throws Exception { .build(); Mockito.when(registry.hasRuleByName(testRuleName)).thenReturn(true); Mockito.when(registry.getRule(testRuleName)).thenReturn(Optional.of(rule)); - Mockito.when(registry.applies(Mockito.any(), Mockito.any())).thenReturn(true); - - ServiceRef serviceRef = - new ServiceRef( - new URI("service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi"), - "io.cryostat.Cryostat"); - Mockito.when(storage.listDiscoverableServices()).thenReturn(List.of(serviceRef)); - JFRConnection connection = Mockito.mock(JFRConnection.class); - Mockito.when(targetConnectionManager.executeConnectedTask(Mockito.any(), Mockito.any())) - .thenAnswer( - arg0 -> - ((TargetConnectionManager.ConnectedTask) - arg0.getArgument(1)) - .execute(connection)); - - IFlightRecorderService service = Mockito.mock(IFlightRecorderService.class); - Mockito.when(connection.getService()).thenReturn(service); - - Mockito.when(service.getAvailableRecordings()).thenReturn(List.of()); + Mockito.when(storage.listDiscoverableServices()).thenReturn(List.of()); IntermediateResponse response = handler.handle(params); MatcherAssert.assertThat(response.getStatusCode(), Matchers.equalTo(200)); - } - @Test - void shouldRespondWith200AfterSuccessfulCleanup() throws Exception { - Mockito.when(params.getPathParams()).thenReturn(Map.of("name", testRuleName)); - MultiMap queryParams = MultiMap.caseInsensitiveMultiMap(); - queryParams.set("clean", "true"); - Mockito.when(params.getQueryParams()).thenReturn(queryParams); - - Rule rule = - new Rule.Builder() - .name(testRuleName) - .matchExpression("true") - .eventSpecifier("template=Continuous") - .build(); - Mockito.when(registry.hasRuleByName(testRuleName)).thenReturn(true); - Mockito.when(registry.getRule(testRuleName)).thenReturn(Optional.of(rule)); - Mockito.when(registry.applies(Mockito.any(), Mockito.any())).thenReturn(true); - - ServiceRef serviceRef = - new ServiceRef( - new URI("service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi"), - "io.cryostat.Cryostat"); - Mockito.when(storage.listDiscoverableServices()).thenReturn(List.of(serviceRef)); - - JFRConnection connection = Mockito.mock(JFRConnection.class); - Mockito.when(targetConnectionManager.executeConnectedTask(Mockito.any(), Mockito.any())) - .thenAnswer( - arg0 -> - ((TargetConnectionManager.ConnectedTask) - arg0.getArgument(1)) - .execute(connection)); - - IFlightRecorderService service = Mockito.mock(IFlightRecorderService.class); - Mockito.when(connection.getService()).thenReturn(service); - - IRecordingDescriptor recording = Mockito.mock(IRecordingDescriptor.class); - Mockito.when(service.getAvailableRecordings()).thenReturn(List.of(recording)); - Mockito.when(recording.getName()).thenReturn(rule.getRecordingName()); - - IntermediateResponse response = handler.handle(params); - MatcherAssert.assertThat(response.getStatusCode(), Matchers.equalTo(200)); - - Mockito.verify(service).stop(recording); - Mockito.verify(service, Mockito.never()).close(recording); + Mockito.verify(vertx, Mockito.times(1)).executeBlocking(Mockito.any()); + Mockito.verify(registry).deleteRule(rule); + Mockito.verify(registry, Mockito.never()).applies(Mockito.any(), Mockito.any()); + Mockito.verify(recordingTargetHelper, Mockito.never()) + .stopRecording(Mockito.any(), Mockito.eq(rule.getRecordingName())); } @Test - void shouldRespondWith200AfterUnsuccessfulCleanup() throws Exception { + void shouldRespondWith200AfterSuccessfulCleanup() throws Exception { Mockito.when(params.getPathParams()).thenReturn(Map.of("name", testRuleName)); MultiMap queryParams = MultiMap.caseInsensitiveMultiMap(); queryParams.set("clean", "true"); @@ -350,29 +301,14 @@ void shouldRespondWith200AfterUnsuccessfulCleanup() throws Exception { "io.cryostat.Cryostat"); Mockito.when(storage.listDiscoverableServices()).thenReturn(List.of(serviceRef)); - JFRConnection connection = Mockito.mock(JFRConnection.class); - Mockito.when(targetConnectionManager.executeConnectedTask(Mockito.any(), Mockito.any())) - .thenAnswer( - arg0 -> - ((TargetConnectionManager.ConnectedTask) - arg0.getArgument(1)) - .execute(connection)); - - IFlightRecorderService service = Mockito.mock(IFlightRecorderService.class); - Mockito.when(connection.getService()).thenReturn(service); - - org.openjdk.jmc.rjmx.services.jfr.FlightRecorderException exception = - new org.openjdk.jmc.rjmx.services.jfr.FlightRecorderException("test message"); - Mockito.doThrow(exception).when(service).stop(Mockito.any()); - - IRecordingDescriptor recording = Mockito.mock(IRecordingDescriptor.class); - Mockito.when(service.getAvailableRecordings()).thenReturn(List.of(recording)); - Mockito.when(recording.getName()).thenReturn(rule.getRecordingName()); - IntermediateResponse response = handler.handle(params); MatcherAssert.assertThat(response.getStatusCode(), Matchers.equalTo(200)); - Mockito.verify(logger).error(exception); + Mockito.verify(vertx, Mockito.times(2)).executeBlocking(Mockito.any()); + Mockito.verify(registry).deleteRule(rule); + Mockito.verify(registry).applies(Mockito.any(), Mockito.any()); + Mockito.verify(recordingTargetHelper) + .stopRecording(Mockito.any(), Mockito.eq(rule.getRecordingName())); } } }