Skip to content

Commit

Permalink
fix(reports): tune flags for reliability
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewazores committed Oct 1, 2021
1 parent 9478f12 commit 40af372
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
Expand Down Expand Up @@ -135,8 +136,7 @@ CompletableFuture<Path> exec(Path recording, Path saveFile, Duration timeout)
// 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(
Expand Down Expand Up @@ -205,18 +205,17 @@ 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("-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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -190,12 +193,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("-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, Duration.ofSeconds(10));

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));
}

Expand Down

0 comments on commit 40af372

Please sign in to comment.