From 37771502aecf24c9e429d56f86260587c24519db Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 9 May 2019 06:30:46 -0400 Subject: [PATCH] Remove manual parsing of JVM options (#41962) This commit removes manual parsing of JVM options when calculating ergonomics. This is to avoid a situation that we parse values differently than the JVM would. In fact, we already have a bug along these lines today. It is possible to start the JVM with the same flag multiple times on the command line. In this case, the last value wins. For example, -Xmx1g -Xmx2g would start the JVM with a heap size of two gigabytes. Our JVM ergonomics ignores this possibility and instead the first value is winning! Our strategy to avoid manual parsing of the JVM options is to start the Java command line parser (without actually starting a JVM) by invoking java with the same command line flags as presented and request that the JVM tell us what values it would start with. This ensures that we have the correct values when making ergonomic decisions. Moreover, our strategy also is ignoring ES_JAVA_OPTS which could override the heap size as well leading to incorrect ergonomic choices. This commit address this issue too. --- distribution/src/bin/elasticsearch | 2 +- .../src/bin/elasticsearch-service.bat | Bin 7518 -> 7503 bytes distribution/src/bin/elasticsearch.bat | Bin 1518 -> 1503 bytes .../tools/launchers/JvmErgonomics.java | 113 +++++++++++------- .../tools/launchers/JvmOptionsParser.java | 20 +++- .../tools/launchers/JvmErgonomicsTests.java | 67 ++++++++--- 6 files changed, 137 insertions(+), 65 deletions(-) diff --git a/distribution/src/bin/elasticsearch b/distribution/src/bin/elasticsearch index 8bdea4950cb..6843607efa1 100755 --- a/distribution/src/bin/elasticsearch +++ b/distribution/src/bin/elasticsearch @@ -18,7 +18,7 @@ source "`dirname "$0"`"/elasticsearch-env ES_JVM_OPTIONS="$ES_PATH_CONF"/jvm.options JVM_OPTIONS=`"$JAVA" -cp "$ES_CLASSPATH" org.elasticsearch.tools.launchers.JvmOptionsParser "$ES_JVM_OPTIONS"` -ES_JAVA_OPTS="${JVM_OPTIONS//\$\{ES_TMPDIR\}/$ES_TMPDIR} $ES_JAVA_OPTS" +ES_JAVA_OPTS="${JVM_OPTIONS//\$\{ES_TMPDIR\}/$ES_TMPDIR}" # manual parsing to find out, if process should be detached if ! echo $* | grep -E '(^-d |-d$| -d |--daemonize$|--daemonize )' > /dev/null; then diff --git a/distribution/src/bin/elasticsearch-service.bat b/distribution/src/bin/elasticsearch-service.bat index 7a0be55c4f56527c44efade68e2c4f6bd327cdc8..2f9c280743dfb756725e226c66a39497670f2609 100644 GIT binary patch delta 12 Tcmca-b>3=29?#}lo`(VeCq)H$ delta 27 icmX?ab_s%vn(mt&Y?ynjGQ@a9~eM*;weED3u6 diff --git a/distribution/src/bin/elasticsearch.bat b/distribution/src/bin/elasticsearch.bat index ecbbad826e797d1e07a8b4ee3ae6149add76ebde..f14185ddc4a2737834f81c9f731aa6f98274e831 100644 GIT binary patch delta 24 gcmaFIeV==SCJQ?kS7w@mlIrAx%<`M#SSBz60AH{NE&u=k delta 39 vcmcc5{f>KsCX1MYs%vn(mt&Y?ynjGQuqqcg0pW@|*oxCNKg3?LG@u diff --git a/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java b/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java index 761cd9e1be5..44f60ba2d9f 100644 --- a/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java +++ b/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java @@ -19,24 +19,28 @@ package org.elasticsearch.tools.launchers; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Paths; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Tunes Elasticsearch JVM settings based on inspection of provided JVM options. */ final class JvmErgonomics { - private static final long KB = 1024L; - - private static final long MB = 1024L * 1024L; - - private static final long GB = 1024L * 1024L * 1024L; - private JvmErgonomics() { throw new AssertionError("No instances intended"); @@ -48,48 +52,74 @@ final class JvmErgonomics { * @param userDefinedJvmOptions A list of JVM options that have been defined by the user. * @return A list of additional JVM options to set. */ - static List choose(List userDefinedJvmOptions) { - List ergonomicChoices = new ArrayList<>(); - Long heapSize = extractHeapSize(userDefinedJvmOptions); - Map systemProperties = extractSystemProperties(userDefinedJvmOptions); - if (heapSize != null) { - if (systemProperties.containsKey("io.netty.allocator.type") == false) { - if (heapSize <= 1 * GB) { - ergonomicChoices.add("-Dio.netty.allocator.type=unpooled"); - } else { - ergonomicChoices.add("-Dio.netty.allocator.type=pooled"); - } + static List choose(final List userDefinedJvmOptions) throws InterruptedException, IOException { + final List ergonomicChoices = new ArrayList<>(); + final Map> finalJvmOptions = finalJvmOptions(userDefinedJvmOptions); + final long heapSize = extractHeapSize(finalJvmOptions); + final Map systemProperties = extractSystemProperties(userDefinedJvmOptions); + if (systemProperties.containsKey("io.netty.allocator.type") == false) { + if (heapSize <= 1 << 30) { + ergonomicChoices.add("-Dio.netty.allocator.type=unpooled"); + } else { + ergonomicChoices.add("-Dio.netty.allocator.type=pooled"); } } return ergonomicChoices; } - private static final Pattern MAX_HEAP_SIZE = Pattern.compile("^(-Xmx|-XX:MaxHeapSize=)(?\\d+)(?\\w)?$"); + private static final Pattern OPTION = + Pattern.compile("^\\s*\\S+\\s+(?\\S+)\\s+:?=\\s+(?\\S+)?\\s+\\{[^}]+?\\}\\s+\\{[^}]+}"); + + static Map> finalJvmOptions( + final List userDefinedJvmOptions) throws InterruptedException, IOException { + return Collections.unmodifiableMap(flagsFinal(userDefinedJvmOptions).stream() + .map(OPTION::matcher).filter(Matcher::matches) + .collect(Collectors.toMap(m -> m.group("flag"), m -> Optional.ofNullable(m.group("value"))))); + } + + private static List flagsFinal(final List userDefinedJvmOptions) throws InterruptedException, IOException { + /* + * To deduce the final set of JVM options that Elasticsearch is going to start with, we start a separate Java process with the JVM + * options that we would pass on the command line. For this Java process we will add two additional flags, -XX:+PrintFlagsFinal and + * -version. This causes the Java process that we start to parse the JVM options into their final values, display them on standard + * output, print the version to standard error, and then exit. The JVM itself never bootstraps, and therefore this process is + * lightweight. By doing this, we get the JVM options parsed exactly as the JVM that we are going to execute would parse them + * without having to implement our own JVM option parsing logic. + */ + final String java = Paths.get(System.getProperty("java.home"), "bin", "java").toString(); + final List command = + Collections.unmodifiableList( + Stream.of(Stream.of(java), userDefinedJvmOptions.stream(), Stream.of("-XX:+PrintFlagsFinal"), Stream.of("-version")) + .reduce(Stream::concat) + .get() + .collect(Collectors.toList())); + final Process process = new ProcessBuilder().command(command).start(); + final List output = readLinesFromInputStream(process.getInputStream()); + final List error = readLinesFromInputStream(process.getErrorStream()); + final int status = process.waitFor(); + if (status != 0) { + final String message = String.format( + Locale.ROOT, + "starting java failed with [%d]\noutput:\n%s\nerror:\n%s", + status, + String.join("\n", output), + String.join("\n", error)); + throw new RuntimeException(message); + } else { + return output; + } + } + + private static List readLinesFromInputStream(final InputStream is) throws IOException { + try (InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8); + BufferedReader br = new BufferedReader(isr)) { + return Collections.unmodifiableList(br.lines().collect(Collectors.toList())); + } + } // package private for testing - static Long extractHeapSize(List userDefinedJvmOptions) { - for (String jvmOption : userDefinedJvmOptions) { - final Matcher matcher = MAX_HEAP_SIZE.matcher(jvmOption); - if (matcher.matches()) { - final long size = Long.parseLong(matcher.group("size")); - final String unit = matcher.group("unit"); - if (unit == null) { - return size; - } else { - switch (unit.toLowerCase(Locale.ROOT)) { - case "k": - return size * KB; - case "m": - return size * MB; - case "g": - return size * GB; - default: - throw new IllegalArgumentException("Unknown unit [" + unit + "] for max heap size in [" + jvmOption + "]"); - } - } - } - } - return null; + static Long extractHeapSize(final Map> finalJvmOptions) { + return Long.parseLong(finalJvmOptions.get("MaxHeapSize").get()); } private static final Pattern SYSTEM_PROPERTY = Pattern.compile("^-D(?[\\w+].*?)=(?.*)$"); @@ -105,4 +135,5 @@ final class JvmErgonomics { } return systemProperties; } + } diff --git a/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmOptionsParser.java b/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmOptionsParser.java index d74f106c50b..586be178f0d 100644 --- a/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmOptionsParser.java +++ b/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmOptionsParser.java @@ -19,12 +19,14 @@ package org.elasticsearch.tools.launchers; +import org.elasticsearch.tools.java_version_checker.JavaVersion; + import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; import java.io.Reader; -import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Paths; import java.util.ArrayList; @@ -37,8 +39,7 @@ import java.util.SortedMap; import java.util.TreeMap; import java.util.regex.Matcher; import java.util.regex.Pattern; - -import org.elasticsearch.tools.java_version_checker.JavaVersion; +import java.util.stream.Collectors; /** * Parses JVM options from a file and prints a single line with all JVM options to standard output. @@ -51,14 +52,14 @@ final class JvmOptionsParser { * * @param args the args to the program which should consist of a single option, the path to the JVM options */ - public static void main(final String[] args) throws IOException { + public static void main(final String[] args) throws InterruptedException, IOException { if (args.length != 1) { throw new IllegalArgumentException("expected one argument specifying path to jvm.options but was " + Arrays.toString(args)); } final List jvmOptions = new ArrayList<>(); final SortedMap invalidLines = new TreeMap<>(); try (InputStream is = Files.newInputStream(Paths.get(args[0])); - Reader reader = new InputStreamReader(is, Charset.forName("UTF-8")); + Reader reader = new InputStreamReader(is, StandardCharsets.UTF_8); BufferedReader br = new BufferedReader(reader)) { parse( JavaVersion.majorVersion(JavaVersion.CURRENT), @@ -78,7 +79,14 @@ final class JvmOptionsParser { } if (invalidLines.isEmpty()) { - List ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions); + // now append the JVM options from ES_JAVA_OPTS + final String environmentJvmOptions = System.getenv("ES_JAVA_OPTS"); + if (environmentJvmOptions != null) { + jvmOptions.addAll(Arrays.stream(environmentJvmOptions.split("\\s+")) + .filter(s -> s.trim().isEmpty() == false) + .collect(Collectors.toList())); + } + final List ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions); jvmOptions.addAll(ergonomicJvmOptions); final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(jvmOptions); Launchers.outPrintln(spaceDelimitedJvmOptions); diff --git a/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java b/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java index 4b075d78b70..b5b6699f471 100644 --- a/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java +++ b/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java @@ -19,38 +19,70 @@ package org.elasticsearch.tools.launchers; +import java.io.IOException; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.hasToString; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class JvmErgonomicsTests extends LaunchersTestCase { - public void testExtractValidHeapSize() { - assertEquals(Long.valueOf(1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx1024"))); - assertEquals(Long.valueOf(2L * 1024 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2g"))); - assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx32M"))); - assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-XX:MaxHeapSize=32M"))); + + public void testExtractValidHeapSizeUsingXmx() throws InterruptedException, IOException { + assertThat( + JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2g"))), + equalTo(2L << 30)); } - public void testExtractInvalidHeapSize() { + public void testExtractValidHeapSizeUsingMaxHeapSize() throws InterruptedException, IOException { + assertThat( + JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-XX:MaxHeapSize=2g"))), + equalTo(2L << 30)); + } + + public void testExtractValidHeapSizeNoOptionPresent() throws InterruptedException, IOException { + assertThat( + JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.emptyList())), + greaterThan(0L)); + } + + public void testHeapSizeInvalid() throws InterruptedException, IOException { try { - JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2T")); - fail("Expected IllegalArgumentException to be raised"); - } catch (IllegalArgumentException expected) { - assertEquals("Unknown unit [T] for max heap size in [-Xmx2T]", expected.getMessage()); + JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2Z"))); + fail("expected starting java to fail"); + } catch (final RuntimeException e) { + assertThat(e, hasToString(containsString(("starting java failed")))); + assertThat(e, hasToString(containsString(("Invalid maximum heap size: -Xmx2Z")))); } } - public void testExtractNoHeapSize() { - assertNull("No spaces allowed", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx 1024"))); - assertNull("JVM option is not present", JvmErgonomics.extractHeapSize(Collections.singletonList(""))); - assertNull("Multiple JVM options per line", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xms2g -Xmx2g"))); + public void testHeapSizeTooSmall() throws InterruptedException, IOException { + try { + JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx1024"))); + fail("expected starting java to fail"); + } catch (final RuntimeException e) { + assertThat(e, hasToString(containsString(("starting java failed")))); + assertThat(e, hasToString(containsString(("Too small maximum heap")))); + } + } + + public void testHeapSizeWithSpace() throws InterruptedException, IOException { + try { + JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx 1024"))); + fail("expected starting java to fail"); + } catch (final RuntimeException e) { + assertThat(e, hasToString(containsString(("starting java failed")))); + assertThat(e, hasToString(containsString(("Invalid maximum heap size: -Xmx 1024")))); + } } public void testExtractSystemProperties() { @@ -69,15 +101,16 @@ public class JvmErgonomicsTests extends LaunchersTestCase { assertTrue(parsedSystemProperties.isEmpty()); } - public void testLittleMemoryErgonomicChoices() { + public void testLittleMemoryErgonomicChoices() throws InterruptedException, IOException { String smallHeap = randomFrom(Arrays.asList("64M", "512M", "1024M", "1G")); List expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=unpooled"); assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + smallHeap, "-Xmx" + smallHeap))); } - public void testPlentyMemoryErgonomicChoices() { + public void testPlentyMemoryErgonomicChoices() throws InterruptedException, IOException { String largeHeap = randomFrom(Arrays.asList("1025M", "2048M", "2G", "8G")); List expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=pooled"); assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + largeHeap, "-Xmx" + largeHeap))); } + }