From 32f5b17913cb44c024dac33ad41474fad9bb7569 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 26 Sep 2019 22:56:31 -0400 Subject: [PATCH 1/4] Move ES_TMPDIR substitution into jvm options parser This commit moves the ES_TMPDIR substitution that we do for JVM options into the JVM options parser itself. This solves a problem where the fact that the we do not make the substitution before ergonomics parsing can lead to the JVM that we start for computing the ergonomic values failing to start. Additionally, moving this substitution here enables us to simplify the shell scripts since we do not need to implement this there, and twice for Bash and Windows. --- distribution/src/bin/elasticsearch | 3 +-- distribution/src/bin/elasticsearch-service.bat | 4 ++-- distribution/src/bin/elasticsearch.bat | 4 ++-- .../elasticsearch/tools/launchers/JvmOptionsParser.java | 9 ++++++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/distribution/src/bin/elasticsearch b/distribution/src/bin/elasticsearch index b7ed2b648b76f..53329cc6bad41 100755 --- a/distribution/src/bin/elasticsearch +++ b/distribution/src/bin/elasticsearch @@ -21,8 +21,7 @@ if [ -z "$ES_TMPDIR" ]; then fi 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=`export ES_TMPDIR; "$JAVA" -cp "$ES_CLASSPATH" org.elasticsearch.tools.launchers.JvmOptionsParser "$ES_JVM_OPTIONS"` # 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 fd4d4b666dba8..4ccda28086c83 100644 --- a/distribution/src/bin/elasticsearch-service.bat +++ b/distribution/src/bin/elasticsearch-service.bat @@ -115,8 +115,8 @@ set ES_JVM_OPTIONS=%ES_PATH_CONF%\jvm.options 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!% +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 ES_JAVA_OPTS=%%a +@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%ES_JAVA_OPTS%" if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" ( exit /b 1 diff --git a/distribution/src/bin/elasticsearch.bat b/distribution/src/bin/elasticsearch.bat index 03dc48728d52c..e23a2a0cc3daf 100644 --- a/distribution/src/bin/elasticsearch.bat +++ b/distribution/src/bin/elasticsearch.bat @@ -47,8 +47,8 @@ if not defined ES_TMPDIR ( 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!% +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 ES_JAVA_OPTS=%%a +@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%ES_JAVA_OPTS%" if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" ( exit /b 1 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 7894cab72a1ee..7e65d277a9ddd 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 @@ -87,9 +87,12 @@ public void accept(final int lineNumber, final String line) { .filter(Predicate.not(String::isBlank)) .collect(Collectors.toUnmodifiableList())); } - final List ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions); - jvmOptions.addAll(ergonomicJvmOptions); - final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(jvmOptions); + final String esTmpdir = System.getenv("ES_TMPDIR"); + final List substitutedJvmOptions = + jvmOptions.stream().map(s -> s.replace("${ES_TMPDIR}", esTmpdir)).collect(Collectors.toList()); + final List ergonomicJvmOptions = JvmErgonomics.choose(substitutedJvmOptions); + substitutedJvmOptions.addAll(ergonomicJvmOptions); + final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(substitutedJvmOptions); Launchers.outPrintln(spaceDelimitedJvmOptions); Launchers.exit(0); } else { From 77dd706da2a61d65c9b369f8126ef90ad3ea95ed Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 27 Sep 2019 09:07:51 -0400 Subject: [PATCH 2/4] Fix Windows --- distribution/src/bin/elasticsearch-service.bat | 2 +- distribution/src/bin/elasticsearch.bat | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/distribution/src/bin/elasticsearch-service.bat b/distribution/src/bin/elasticsearch-service.bat index 4ccda28086c83..3b1478110692e 100644 --- a/distribution/src/bin/elasticsearch-service.bat +++ b/distribution/src/bin/elasticsearch-service.bat @@ -116,7 +116,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 ES_JAVA_OPTS=%%a -@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%ES_JAVA_OPTS%" +@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%ES_JAVA_OPTS%" & set ES_JAVA_OPTS=%ES_JAVA_OPTS% if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" ( exit /b 1 diff --git a/distribution/src/bin/elasticsearch.bat b/distribution/src/bin/elasticsearch.bat index e23a2a0cc3daf..9460554f81f41 100644 --- a/distribution/src/bin/elasticsearch.bat +++ b/distribution/src/bin/elasticsearch.bat @@ -48,7 +48,7 @@ if not defined ES_TMPDIR ( 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 ES_JAVA_OPTS=%%a -@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%ES_JAVA_OPTS%" +@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%ES_JAVA_OPTS%" & set ES_JAVA_OPTS=%ES_JAVA_OPTS% if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" ( exit /b 1 From 00ebd8b7f7ec9b23296857088a86b8a67ef6f336 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 27 Sep 2019 12:25:23 -0400 Subject: [PATCH 3/4] Add test --- .../tools/launchers/JvmOptionsParser.java | 21 +++++++++++++++++-- .../launchers/JvmOptionsParserTests.java | 7 +++++++ 2 files changed, 26 insertions(+), 2 deletions(-) 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 7e65d277a9ddd..db972c1a9e9fe 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 @@ -87,9 +87,8 @@ public void accept(final int lineNumber, final String line) { .filter(Predicate.not(String::isBlank)) .collect(Collectors.toUnmodifiableList())); } - final String esTmpdir = System.getenv("ES_TMPDIR"); final List substitutedJvmOptions = - jvmOptions.stream().map(s -> s.replace("${ES_TMPDIR}", esTmpdir)).collect(Collectors.toList()); + substitutePlaceholders(jvmOptions, Map.of("ES_TMPDIR", System.getenv("ES_TMPDIR"))); final List ergonomicJvmOptions = JvmErgonomics.choose(substitutedJvmOptions); substitutedJvmOptions.addAll(ergonomicJvmOptions); final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(substitutedJvmOptions); @@ -118,6 +117,24 @@ public void accept(final int lineNumber, final String line) { } } + static List substitutePlaceholders(final List jvmOptions, Map substitutions) { + final Map placeholderSubstitutions = + substitutions.entrySet().stream().collect(Collectors.toMap(e -> "${" + e.getKey() + "}", Map.Entry::getValue)); + final List substitutedJvmOptions = new ArrayList<>(jvmOptions.size()); + for (final String jvmOption : jvmOptions) { + if (jvmOption.matches(".*\\$\\{[^}]+\\}.*")) { + String actualJvmOption = jvmOption; + for (final Map.Entry placeholderSubstitution : placeholderSubstitutions.entrySet()) { + actualJvmOption = actualJvmOption.replace(placeholderSubstitution.getKey(), placeholderSubstitution.getValue()); + } + substitutedJvmOptions.add(actualJvmOption); + } else { + substitutedJvmOptions.add(jvmOption); + } + } + return substitutedJvmOptions; + } + /** * Callback for valid JVM options. */ diff --git a/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmOptionsParserTests.java b/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmOptionsParserTests.java index 1433d19e8bb65..41a2b71847e2d 100644 --- a/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmOptionsParserTests.java +++ b/distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmOptionsParserTests.java @@ -30,6 +30,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; @@ -39,6 +40,12 @@ public class JvmOptionsParserTests extends LaunchersTestCase { + public void testSubstitution() { + final List jvmOptions = + JvmOptionsParser.substitutePlaceholders(List.of("-Djava.io.tmpdir=${ES_TMPDIR}"), Map.of("ES_TMPDIR", "/tmp/elasticsearch")); + assertThat(jvmOptions, contains("-Djava.io.tmpdir=/tmp/elasticsearch")); + } + public void testUnversionedOptions() throws IOException { try (StringReader sr = new StringReader("-Xms1g\n-Xmx1g"); BufferedReader br = new BufferedReader(sr)) { From 4844dc2084bdc11be13e7d45bd0fc672bf4832af Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 4 Oct 2019 10:50:36 -0400 Subject: [PATCH 4/4] Iteration --- .../tools/launchers/JvmOptionsParser.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) 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 db972c1a9e9fe..757a1b3987f24 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 @@ -117,22 +117,22 @@ public void accept(final int lineNumber, final String line) { } } - static List substitutePlaceholders(final List jvmOptions, Map substitutions) { + static List substitutePlaceholders(final List jvmOptions, final Map substitutions) { final Map placeholderSubstitutions = substitutions.entrySet().stream().collect(Collectors.toMap(e -> "${" + e.getKey() + "}", Map.Entry::getValue)); - final List substitutedJvmOptions = new ArrayList<>(jvmOptions.size()); - for (final String jvmOption : jvmOptions) { - if (jvmOption.matches(".*\\$\\{[^}]+\\}.*")) { - String actualJvmOption = jvmOption; - for (final Map.Entry placeholderSubstitution : placeholderSubstitutions.entrySet()) { - actualJvmOption = actualJvmOption.replace(placeholderSubstitution.getKey(), placeholderSubstitution.getValue()); - } - substitutedJvmOptions.add(actualJvmOption); - } else { - substitutedJvmOptions.add(jvmOption); - } - } - return substitutedJvmOptions; + return jvmOptions.stream() + .map( + jvmOption -> { + String actualJvmOption = jvmOption; + int start = jvmOption.indexOf("${"); + if (start >= 0 && jvmOption.indexOf('}', start) > 0) { + for (final Map.Entry placeholderSubstitution : placeholderSubstitutions.entrySet()) { + actualJvmOption = actualJvmOption.replace(placeholderSubstitution.getKey(), placeholderSubstitution.getValue()); + } + } + return actualJvmOption; + }) + .collect(Collectors.toList()); } /**