diff --git a/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java b/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java index 0fdf3d3724..488818075c 100644 --- a/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java +++ b/src/main/java/io/cryostat/net/reports/SubprocessReportGenerator.java @@ -44,6 +44,7 @@ import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -79,8 +80,6 @@ public class SubprocessReportGenerator { static final String SUBPROCESS_MAX_HEAP_ENV = "CRYOSTAT_REPORT_GENERATION_MAX_HEAP"; - static String ENV_USERNAME = "TARGET_USERNAME"; - static String ENV_PASSWORD = "TARGET_PASSWORD"; private final Environment env; private final FileSystem fs; @@ -134,15 +133,17 @@ CompletableFuture exec(Path recording, Path saveFile) // See https://github.com/cryostatio/cryostat/issues/287 .jvmArgs( createJvmArgs( - Integer.parseInt( - env.getEnv(SUBPROCESS_MAX_HEAP_ENV, "200")))) + Integer.parseInt(env.getEnv(SUBPROCESS_MAX_HEAP_ENV, "0")))) .processArgs(createProcessArgs(recording, saveFile)) .exec(); return CompletableFuture.supplyAsync( () -> { try { proc.waitFor(5, TimeUnit.MINUTES); - ExitStatus status = ExitStatus.byExitCode(proc.exitValue()); + ExitStatus status = + proc.isAlive() + ? ExitStatus.TIMED_OUT + : ExitStatus.byExitCode(proc.exitValue()); switch (status) { case OK: return saveFile; @@ -157,7 +158,9 @@ CompletableFuture exec(Path recording, Path saveFile) proc.destroyForcibly(); throw new CompletionException( new ReportGenerationException(ExitStatus.TERMINATED)); - } catch (ReportGenerationException | RecordingNotFoundException e) { + } catch (ReportGenerationException + | RecordingNotFoundException + | IllegalThreadStateException e) { logger.error(e); proc.destroyForcibly(); throw new CompletionException(e); @@ -204,18 +207,18 @@ Path copyRecordingToFile(JFRConnection conn, String recordingName, Path path) th } private List createJvmArgs(int maxHeapMegabytes) throws IOException { - // These JVM flags must be kept in-sync with the flags set on the parent process in - // entrypoint.sh in order to keep the auth and certs setup consistent - return List.of( - String.format("-Xmx%dM", maxHeapMegabytes), - "-XX:+ExitOnOutOfMemoryError", - // use EpsilonGC since we're a one-shot process and report generation doesn't - // allocate that much memory beyond the initial recording load - no point in - // over-complicating memory allocation. Preferable to just complete the request - // quickly, or if we're running up against the memory limit, fail early - "-XX:+UnlockExperimentalVMOptions", - "-XX:+UseEpsilonGC", - "-XX:+AlwaysPreTouch"); + List args = new ArrayList<>(); + if (maxHeapMegabytes > 0) { + args.add(String.format("-Xms%dM", maxHeapMegabytes)); + args.add(String.format("-Xmx%dM", maxHeapMegabytes)); + } + args.add("-XX:+ExitOnOutOfMemoryError"); + // use Serial GC since we have a small heap and likely little garbage to clean, + // and low GC overhead is more important here than minimizing pause time since the + // result will end up cached for subsequent user accesses so long as the process + // succeeds in the end + args.add("-XX:+UseSerialGC"); + return args; } private List createProcessArgs(Path recording, Path saveFile) { @@ -382,7 +385,7 @@ public enum ExitStatus { IO_EXCEPTION(5, "An unspecified IO exception occurred while writing the report file."), OTHER(6, "An unspecified unexpected exception occurred."), TERMINATED(-1, "The subprocess timed out and was terminated."), - ; + TIMED_OUT(-2, "The subprocess did not complete its work within the allotted time."); final int code; final String message; diff --git a/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java b/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java index 2af3892bd2..f2214a8b38 100644 --- a/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java +++ b/src/test/java/io/cryostat/net/reports/SubprocessReportGeneratorTest.java @@ -118,7 +118,10 @@ void setup() throws Exception { .thenReturn(javaProcessBuilder); Mockito.lenient().when(javaProcessBuilder.exec()).thenReturn(proc); Mockito.lenient() - .when(env.getEnv(SubprocessReportGenerator.SUBPROCESS_MAX_HEAP_ENV, "200")) + .when( + env.getEnv( + Mockito.eq(SubprocessReportGenerator.SUBPROCESS_MAX_HEAP_ENV), + Mockito.anyString())) .thenReturn("200"); this.generator = new SubprocessReportGenerator( @@ -189,12 +192,27 @@ void shouldSetJvmArgs() throws Exception { Mockito.verify(javaProcessBuilder).jvmArgs(captor.capture()); List expected = - List.of( - "-Xmx200M", - "-XX:+ExitOnOutOfMemoryError", - "-XX:+UnlockExperimentalVMOptions", - "-XX:+UseEpsilonGC", - "-XX:+AlwaysPreTouch"); + List.of("-Xms200M", "-Xmx200M", "-XX:+ExitOnOutOfMemoryError", "-XX:+UseSerialGC"); + MatcherAssert.assertThat(captor.getValue(), Matchers.equalTo(expected)); + } + + @Test + void shouldSetJvmArgsWithoutReportMaxHeapEnvVar() throws Exception { + Path dest = Mockito.mock(Path.class); + Mockito.when(dest.toAbsolutePath()).thenReturn(dest); + Mockito.when(dest.toString()).thenReturn("/dest/somefile.tmp"); + Mockito.when( + env.getEnv( + Mockito.eq(SubprocessReportGenerator.SUBPROCESS_MAX_HEAP_ENV), + Mockito.anyString())) + .thenReturn("0"); + + generator.exec(recordingFile, dest); + + ArgumentCaptor> captor = ArgumentCaptor.forClass(List.class); + Mockito.verify(javaProcessBuilder).jvmArgs(captor.capture()); + + List expected = List.of("-XX:+ExitOnOutOfMemoryError", "-XX:+UseSerialGC"); MatcherAssert.assertThat(captor.getValue(), Matchers.equalTo(expected)); }