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

@@ -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

distribution/src/bin/elasticsearch-service.bat

@@ -112,7 +112,7 @@ if not "%ES_JAVA_OPTS%" == "" set ES_JAVA_OPTS=%ES_JAVA_OPTS: =;%
 
 @setlocal
 for /F "usebackq delims=" %%a in (`"%JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.JvmOptionsParser" "!ES_JVM_OPTIONS!" || echo jvm_options_parser_failed"`) do set JVM_OPTIONS=%%a
-@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!% %ES_JAVA_OPTS%
+@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!%
 
 if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" (
   exit /b 1

distribution/src/bin/elasticsearch.bat

@@ -44,7 +44,7 @@ IF ERRORLEVEL 1 (
 set ES_JVM_OPTIONS=%ES_PATH_CONF%\jvm.options
 @setlocal
 for /F "usebackq delims=" %%a in (`CALL %JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.JvmOptionsParser" "!ES_JVM_OPTIONS!" ^|^| echo jvm_options_parser_failed`) do set JVM_OPTIONS=%%a
-@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!% %ES_JAVA_OPTS%
+@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!%
 
 if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" (
   exit /b 1

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<String> choose(List<String> userDefinedJvmOptions) {
+    static List<String> choose(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
-        List<String> ergonomicChoices = new ArrayList<>();
+        final List<String> ergonomicChoices = new ArrayList<>();
-        Long heapSize = extractHeapSize(userDefinedJvmOptions);
+        final Map<String, Optional<String>> finalJvmOptions = finalJvmOptions(userDefinedJvmOptions);
-        Map<String, String> systemProperties = extractSystemProperties(userDefinedJvmOptions);
+        final long heapSize = extractHeapSize(finalJvmOptions);
-        if (heapSize != null) {
+        final Map<String, String> systemProperties = extractSystemProperties(userDefinedJvmOptions);
-            if (systemProperties.containsKey("io.netty.allocator.type") == false) {
+        if (systemProperties.containsKey("io.netty.allocator.type") == false) {
-                if (heapSize <= 1 * GB) {
+            if (heapSize <= 1 << 30) {
-                    ergonomicChoices.add("-Dio.netty.allocator.type=unpooled");
+                ergonomicChoices.add("-Dio.netty.allocator.type=unpooled");
-                } else {
+            } else {
-                    ergonomicChoices.add("-Dio.netty.allocator.type=pooled");
+                ergonomicChoices.add("-Dio.netty.allocator.type=pooled");
-                }
             }
         }
         return ergonomicChoices;
     }
 
-    private static final Pattern MAX_HEAP_SIZE = Pattern.compile("^(-Xmx|-XX:MaxHeapSize=)(?<size>\\d+)(?<unit>\\w)?$");
+    private static final Pattern OPTION =
+            Pattern.compile("^\\s*\\S+\\s+(?<flag>\\S+)\\s+:?=\\s+(?<value>\\S+)?\\s+\\{[^}]+?\\}\\s+\\{[^}]+}");
+
+    static Map<String, Optional<String>> finalJvmOptions(
+            final List<String> 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<String> flagsFinal(final List<String> 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<String> 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<String> output = readLinesFromInputStream(process.getInputStream());
+        final List<String> 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<String> 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<String> userDefinedJvmOptions) {
+    static Long extractHeapSize(final Map<String, Optional<String>> finalJvmOptions) {
-        for (String jvmOption : userDefinedJvmOptions) {
+        return Long.parseLong(finalJvmOptions.get("MaxHeapSize").get());
-            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;
     }
 
     private static final Pattern SYSTEM_PROPERTY = Pattern.compile("^-D(?<key>[\\w+].*?)=(?<value>.*)$");
@@ -105,4 +135,5 @@ final class JvmErgonomics {
         }
         return systemProperties;
     }
+
 }

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 java.util.stream.Collectors;
-import org.elasticsearch.tools.java_version_checker.JavaVersion;
 
 /**
  * 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<String> jvmOptions = new ArrayList<>();
         final SortedMap<Integer, String> 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<String> 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<String> ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions);
             jvmOptions.addAll(ergonomicJvmOptions);
             final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(jvmOptions);
             Launchers.outPrintln(spaceDelimitedJvmOptions);

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")));
+    public void testExtractValidHeapSizeUsingXmx() throws InterruptedException, IOException {
-        assertEquals(Long.valueOf(2L * 1024 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2g")));
+        assertThat(
-        assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx32M")));
+                JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2g"))),
-        assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-XX:MaxHeapSize=32M")));
+                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"));
+            JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2Z")));
-            fail("Expected IllegalArgumentException to be raised");
+            fail("expected starting java to fail");
-        } catch (IllegalArgumentException expected) {
+        } catch (final RuntimeException e) {
-            assertEquals("Unknown unit [T] for max heap size in [-Xmx2T]", expected.getMessage());
+            assertThat(e, hasToString(containsString(("starting java failed"))));
+            assertThat(e, hasToString(containsString(("Invalid maximum heap size: -Xmx2Z"))));
         }
     }
 
-    public void testExtractNoHeapSize() {
+    public void testHeapSizeTooSmall() throws InterruptedException, IOException {
-        assertNull("No spaces allowed", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx 1024")));
+        try {
-        assertNull("JVM option is not present", JvmErgonomics.extractHeapSize(Collections.singletonList("")));
+            JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx1024")));
-        assertNull("Multiple JVM options per line", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xms2g -Xmx2g")));
+            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<String> 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<String> expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=pooled");
         assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + largeHeap, "-Xmx" + largeHeap)));
     }
+
 }