fix(reports): tune flags for reliability (backport #698) (#747)

* fix(reports): tune flags for reliability (#698)

* fix(reports): tune flags for reliability

* chore(run.sh): default to 512MB memory

* fix(timeouthandler): increase default timeout to 30s

* fix(reports): improve subprocess timeout handling

* chore(reports): remove unused constants

(cherry picked from commit c4cc64b)

* fixup! fix(reports): tune flags for reliability (#698)

Co-authored-by: Andrew Azores <aazores@redhat.com>

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<Path> 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<Path> 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<String> 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<String> 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<String> 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;

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<String> 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<List<String>> captor = ArgumentCaptor.forClass(List.class);
+        Mockito.verify(javaProcessBuilder).jvmArgs(captor.capture());
+
+        List<String> expected = List.of("-XX:+ExitOnOutOfMemoryError", "-XX:+UseSerialGC");
         MatcherAssert.assertThat(captor.getValue(), Matchers.equalTo(expected));
     }