Add unit tests for reading JVM options files (#52176)

This commit adds some unit tests to cover the reading of JVM options
files.

Author: jasontedor

distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmOptionsParser.java

@@ -50,7 +50,7 @@
  */
 final class JvmOptionsParser {
 
-    private static class JvmOptionsFileParserException extends Exception {
+    static class JvmOptionsFileParserException extends Exception {
 
         private final Path jvmOptionsFile;
 
@@ -126,6 +126,29 @@ private List<String> jvmOptions(final Path config, final String esJavaOpts, fina
         throws InterruptedException,
         IOException,
         JvmOptionsFileParserException {
+
+        final List<String> jvmOptions = readJvmOptionsFiles(config);
+
+        if (esJavaOpts != null) {
+            jvmOptions.addAll(
+                Arrays.stream(esJavaOpts.split("\\s+")).filter(s -> s.trim().isEmpty() == false).collect(Collectors.toList())
+            );
+        }
+
+        final List<String> substitutedJvmOptions = substitutePlaceholders(jvmOptions, Collections.unmodifiableMap(substitutions));
+        final List<String> ergonomicJvmOptions = JvmErgonomics.choose(substitutedJvmOptions);
+        final List<String> systemJvmOptions = SystemJvmOptions.systemJvmOptions();
+        final List<String> finalJvmOptions = new ArrayList<>(
+            systemJvmOptions.size() + substitutedJvmOptions.size() + ergonomicJvmOptions.size()
+        );
+        finalJvmOptions.addAll(systemJvmOptions); // add the system JVM options first so that they can be overridden
+        finalJvmOptions.addAll(substitutedJvmOptions);
+        finalJvmOptions.addAll(ergonomicJvmOptions);
+
+        return finalJvmOptions;
+    }
+
+    List<String> readJvmOptionsFiles(final Path config) throws IOException, JvmOptionsFileParserException {
         final ArrayList<Path> jvmOptionsFiles = new ArrayList<>();
         jvmOptionsFiles.add(config.resolve("jvm.options"));
 
@@ -154,23 +177,7 @@ private List<String> jvmOptions(final Path config, final String esJavaOpts, fina
             }
         }
 
-        if (esJavaOpts != null) {
-            jvmOptions.addAll(
-                Arrays.stream(esJavaOpts.split("\\s+")).filter(s -> s.trim().isEmpty() == false).collect(Collectors.toList())
-            );
-        }
-
-        final List<String> substitutedJvmOptions = substitutePlaceholders(jvmOptions, Collections.unmodifiableMap(substitutions));
-        final List<String> ergonomicJvmOptions = JvmErgonomics.choose(substitutedJvmOptions);
-        final List<String> systemJvmOptions = SystemJvmOptions.systemJvmOptions();
-        final List<String> finalJvmOptions = new ArrayList<>(
-            systemJvmOptions.size() + substitutedJvmOptions.size() + ergonomicJvmOptions.size()
-        );
-        finalJvmOptions.addAll(systemJvmOptions); // add the system JVM options first so that they can be overridden
-        finalJvmOptions.addAll(substitutedJvmOptions);
-        finalJvmOptions.addAll(ergonomicJvmOptions);
-
-        return finalJvmOptions;
+        return jvmOptions;
     }
 
     static List<String> substitutePlaceholders(final List<String> jvmOptions, final Map<String, String> substitutions) {

distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmOptionsParserTests.java

@@ -22,6 +22,10 @@
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.StringReader;
+import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
@@ -31,7 +35,10 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasKey;
+import static org.hamcrest.Matchers.hasSize;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
@@ -149,6 +156,113 @@ public void testComplexOptions() throws IOException {
         }
     }
 
+    public void testMissingRootJvmOptions() throws IOException, JvmOptionsParser.JvmOptionsFileParserException {
+        final Path config = newTempDir();
+        try {
+            final JvmOptionsParser parser = new JvmOptionsParser();
+            parser.readJvmOptionsFiles(config);
+            fail("expected no such file exception, the root jvm.options file does not exist");
+        } catch (final NoSuchFileException expected) {
+            // this is expected, the root JVM options file must exist
+        }
+    }
+
+    public void testReadRootJvmOptions() throws IOException, JvmOptionsParser.JvmOptionsFileParserException {
+        final Path config = newTempDir();
+        final Path rootJvmOptions = config.resolve("jvm.options");
+        Files.write(
+            rootJvmOptions,
+            Arrays.asList("# comment", "-Xms256m", "-Xmx256m"),
+            StandardOpenOption.CREATE_NEW,
+            StandardOpenOption.APPEND
+        );
+        if (randomBoolean()) {
+            // an empty jvm.options.d directory should be irrelevant
+            Files.createDirectory(config.resolve("jvm.options.d"));
+        }
+        final JvmOptionsParser parser = new JvmOptionsParser();
+        final List<String> jvmOptions = parser.readJvmOptionsFiles(config);
+        assertThat(jvmOptions, contains("-Xms256m", "-Xmx256m"));
+    }
+
+    public void testReadJvmOptionsDirectory() throws IOException, JvmOptionsParser.JvmOptionsFileParserException {
+        final Path config = newTempDir();
+        Files.createDirectory(config.resolve("jvm.options.d"));
+        Files.write(
+            config.resolve("jvm.options"),
+            Arrays.asList("# comment", "-Xms256m", "-Xmx256m"),
+            StandardOpenOption.CREATE_NEW,
+            StandardOpenOption.APPEND
+        );
+        Files.write(
+            config.resolve("jvm.options.d").resolve("heap.options"),
+            Arrays.asList("# comment", "-Xms384m", "-Xmx384m"),
+            StandardOpenOption.CREATE_NEW,
+            StandardOpenOption.APPEND
+        );
+        final JvmOptionsParser parser = new JvmOptionsParser();
+        final List<String> jvmOptions = parser.readJvmOptionsFiles(config);
+        assertThat(jvmOptions, contains("-Xms256m", "-Xmx256m", "-Xms384m", "-Xmx384m"));
+    }
+
+    public void testReadJvmOptionsDirectoryInOrder() throws IOException, JvmOptionsParser.JvmOptionsFileParserException {
+        final Path config = newTempDir();
+        Files.createDirectory(config.resolve("jvm.options.d"));
+        Files.write(
+            config.resolve("jvm.options"),
+            Arrays.asList("# comment", "-Xms256m", "-Xmx256m"),
+            StandardOpenOption.CREATE_NEW,
+            StandardOpenOption.APPEND
+        );
+        Files.write(
+            config.resolve("jvm.options.d").resolve("first.options"),
+            Arrays.asList("# comment", "-Xms384m", "-Xmx384m"),
+            StandardOpenOption.CREATE_NEW,
+            StandardOpenOption.APPEND
+        );
+        Files.write(
+            config.resolve("jvm.options.d").resolve("second.options"),
+            Arrays.asList("# comment", "-Xms512m", "-Xmx512m"),
+            StandardOpenOption.CREATE_NEW,
+            StandardOpenOption.APPEND
+        );
+        final JvmOptionsParser parser = new JvmOptionsParser();
+        final List<String> jvmOptions = parser.readJvmOptionsFiles(config);
+        assertThat(jvmOptions, contains("-Xms256m", "-Xmx256m", "-Xms384m", "-Xmx384m", "-Xms512m", "-Xmx512m"));
+    }
+
+    public void testReadJvmOptionsDirectoryIgnoresFilesNotNamedOptions() throws IOException,
+        JvmOptionsParser.JvmOptionsFileParserException {
+        final Path config = newTempDir();
+        Files.createFile(config.resolve("jvm.options"));
+        Files.createDirectory(config.resolve("jvm.options.d"));
+        Files.write(
+            config.resolve("jvm.options.d").resolve("heap.not-named-options"),
+            Arrays.asList("# comment", "-Xms256m", "-Xmx256m"),
+            StandardOpenOption.CREATE_NEW,
+            StandardOpenOption.APPEND
+        );
+        final JvmOptionsParser parser = new JvmOptionsParser();
+        final List<String> jvmOptions = parser.readJvmOptionsFiles(config);
+        assertThat(jvmOptions, empty());
+    }
+
+    public void testFileContainsInvalidLinesThrowsParserException() throws IOException {
+        final Path config = newTempDir();
+        final Path rootJvmOptions = config.resolve("jvm.options");
+        Files.write(rootJvmOptions, Arrays.asList("XX:+UseG1GC"), StandardOpenOption.CREATE_NEW, StandardOpenOption.APPEND);
+        try {
+            final JvmOptionsParser parser = new JvmOptionsParser();
+            parser.readJvmOptionsFiles(config);
+            fail("expected JVM options file parser exception, XX:+UseG1GC is improperly formatted");
+        } catch (final JvmOptionsParser.JvmOptionsFileParserException expected) {
+            assertThat(expected.jvmOptionsFile(), equalTo(rootJvmOptions));
+            assertThat(expected.invalidLines().entrySet(), hasSize(1));
+            assertThat(expected.invalidLines(), hasKey(1));
+            assertThat(expected.invalidLines().get(1), equalTo("XX:+UseG1GC"));
+        }
+    }
+
     private void assertExpectedJvmOptions(final int javaMajorVersion, final BufferedReader br, final List<String> expectedJvmOptions)
         throws IOException {
         final Map<String, AtomicBoolean> seenJvmOptions = new HashMap<>();