From 79921bfec84ca91331be00ff404f8518fd4749a8 Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 30 Jan 2023 17:01:37 +0000 Subject: [PATCH 1/3] Fix some possible NPEs in strange JVM configs `JvmErgonomics` requires various JVM options to be present, but if they are omitted then we throw a `NullPointerException` which looks to the user like an ES bug. They would have to be doing something a little odd to get into this state, but nonetheless it is possible to hit these NPEs. We don't need to handle such a config gracefully, but we should clarify why Elasticsearch won't start to help the user fix their config. --- .../server/cli/JvmErgonomics.java | 33 ++++++++------ .../elasticsearch/server/cli/JvmOption.java | 7 +++ .../server/cli/JvmErgonomicsTests.java | 43 +++++++++++++++++++ 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java index 46e3da3ced90b..f32f9def28617 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java @@ -64,28 +64,33 @@ static boolean tuneG1GCForSmallHeap(final long heapSize) { } static boolean tuneG1GCHeapRegion(final Map finalJvmOptions, final boolean tuneG1GCForSmallHeap) { - JvmOption g1GCHeapRegion = finalJvmOptions.get("G1HeapRegionSize"); - JvmOption g1GC = finalJvmOptions.get("UseG1GC"); - return (tuneG1GCForSmallHeap && g1GC.getMandatoryValue().equals("true") && g1GCHeapRegion.isCommandLineOrigin() == false); + return tuneG1GCForSmallHeap && usingG1GcWithoutCommandLineOriginOption(finalJvmOptions, "G1HeapRegionSize"); } static int tuneG1GCReservePercent(final Map finalJvmOptions, final boolean tuneG1GCForSmallHeap) { - JvmOption g1GC = finalJvmOptions.get("UseG1GC"); - JvmOption g1GCReservePercent = finalJvmOptions.get("G1ReservePercent"); - if (g1GC.getMandatoryValue().equals("true")) { - if (g1GCReservePercent.isCommandLineOrigin() == false && tuneG1GCForSmallHeap) { - return 15; - } else if (g1GCReservePercent.isCommandLineOrigin() == false && tuneG1GCForSmallHeap == false) { - return 25; - } + if (usingG1GcWithoutCommandLineOriginOption(finalJvmOptions, "G1ReservePercent")) { + return tuneG1GCForSmallHeap ? 15 : 25; } return 0; } static boolean tuneG1GCInitiatingHeapOccupancyPercent(final Map finalJvmOptions) { - JvmOption g1GC = finalJvmOptions.get("UseG1GC"); - JvmOption g1GCInitiatingHeapOccupancyPercent = finalJvmOptions.get("InitiatingHeapOccupancyPercent"); - return g1GCInitiatingHeapOccupancyPercent.isCommandLineOrigin() == false && g1GC.getMandatoryValue().equals("true"); + return usingG1GcWithoutCommandLineOriginOption(finalJvmOptions, "InitiatingHeapOccupancyPercent"); + } + + private static boolean usingG1GcWithoutCommandLineOriginOption(Map finalJvmOptions, String optionName) { + return getRequiredOption(finalJvmOptions, "UseG1GC").getMandatoryValue().equals("true") + && getRequiredOption(finalJvmOptions, optionName).isCommandLineOrigin() == false; + } + + private static JvmOption getRequiredOption(final Map finalJvmOptions, final String key) { + final var jvmOption = finalJvmOptions.get(key); + if (jvmOption == null) { + throw new IllegalStateException( + "JVM option [" + key + "] was unexpectedly missing. Elasticsearch requires this option to be present." + ); + } + return jvmOption; } private static final Pattern SYSTEM_PROPERTY = Pattern.compile("^-D(?[\\w+].*?)=(?.*)$"); diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmOption.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmOption.java index 39bf2e54dade0..60cbcb86c02b9 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmOption.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmOption.java @@ -8,6 +8,8 @@ package org.elasticsearch.server.cli; +import org.elasticsearch.common.Strings; + import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; @@ -29,6 +31,11 @@ class JvmOption { private final String origin; JvmOption(String value, String origin) { + if (origin == null) { + throw new IllegalStateException(Strings.format(""" + Elasticsearch could not determine the origin of JVM option [%s]. \ + This indicates that it is running in an unsupported configuration.""", value)); + } this.value = value; this.origin = origin; } diff --git a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/JvmErgonomicsTests.java b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/JvmErgonomicsTests.java index f68a51de85c2a..0d4edfc384d46 100644 --- a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/JvmErgonomicsTests.java +++ b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/JvmErgonomicsTests.java @@ -19,6 +19,7 @@ import java.util.List; import java.util.Map; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.everyItem; @@ -179,4 +180,46 @@ public void testMaxDirectMemorySizeChoiceWhenSet() throws Exception { ); } + @SuppressWarnings("ConstantConditions") + public void testMissingOptionHandling() { + final Map g1GcOn = Map.of("UseG1GC", new JvmOption("true", "")); + final Map g1GcOff = Map.of("UseG1GC", new JvmOption("", "")); + + assertFalse(JvmErgonomics.tuneG1GCHeapRegion(Map.of(), false)); + assertThat( + expectThrows(IllegalStateException.class, () -> JvmErgonomics.tuneG1GCHeapRegion(Map.of(), true)).getMessage(), + allOf(containsString("[UseG1GC]"), containsString("unexpectedly missing")) + ); + assertThat( + expectThrows(IllegalStateException.class, () -> JvmErgonomics.tuneG1GCHeapRegion(g1GcOn, true)).getMessage(), + allOf(containsString("[G1HeapRegionSize]"), containsString("unexpectedly missing")) + ); + assertFalse(JvmErgonomics.tuneG1GCHeapRegion(g1GcOff, randomBoolean())); + + assertThat( + expectThrows(IllegalStateException.class, () -> JvmErgonomics.tuneG1GCReservePercent(Map.of(), randomBoolean())).getMessage(), + allOf(containsString("[UseG1GC]"), containsString("unexpectedly missing")) + ); + assertThat( + expectThrows(IllegalStateException.class, () -> JvmErgonomics.tuneG1GCReservePercent(g1GcOn, randomBoolean())).getMessage(), + allOf(containsString("[G1ReservePercent]"), containsString("unexpectedly missing")) + ); + assertEquals(0, JvmErgonomics.tuneG1GCReservePercent(g1GcOff, randomBoolean())); + + assertThat( + expectThrows(IllegalStateException.class, () -> JvmErgonomics.tuneG1GCInitiatingHeapOccupancyPercent(Map.of())).getMessage(), + allOf(containsString("[UseG1GC]"), containsString("unexpectedly missing")) + ); + assertThat( + expectThrows(IllegalStateException.class, () -> JvmErgonomics.tuneG1GCInitiatingHeapOccupancyPercent(g1GcOn)).getMessage(), + allOf(containsString("[InitiatingHeapOccupancyPercent]"), containsString("unexpectedly missing")) + ); + assertFalse(JvmErgonomics.tuneG1GCInitiatingHeapOccupancyPercent(g1GcOff)); + + assertThat( + expectThrows(IllegalStateException.class, () -> new JvmOption("OptionName", null)).getMessage(), + allOf(containsString("could not determine the origin of JVM option [OptionName]"), containsString("unsupported")) + ); + } + } From 6585bf490364e0b4838dec9ff2db5d780237d8df Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 30 Jan 2023 17:32:46 +0000 Subject: [PATCH 2/3] Add Javadocs --- .../java/org/elasticsearch/server/cli/JvmErgonomics.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java index f32f9def28617..3af0031df7c56 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java @@ -78,6 +78,15 @@ static boolean tuneG1GCInitiatingHeapOccupancyPercent(final Map + *
  • {@code true} if `-XX:+UseG1GC` is in the final JVM options and {@code optionName} was not specified. + *
  • {@code false} if either `-XX:-UseG1GC` is in the final JVM options, or {@code optionName} was specified. + * + * + * @throws IllegalStateException if neither `-XX:+UseG1GC` nor `-XX:-UseG1GC` is in the final JVM options, or `-XX:+UseG1GC` is selected + * and {@code optionName} is missing. + */ private static boolean usingG1GcWithoutCommandLineOriginOption(Map finalJvmOptions, String optionName) { return getRequiredOption(finalJvmOptions, "UseG1GC").getMandatoryValue().equals("true") && getRequiredOption(finalJvmOptions, optionName).isCommandLineOrigin() == false; From 1f3a3b6c500cd8a0f7f00bbc362b78d067d75325 Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 30 Jan 2023 17:34:28 +0000 Subject: [PATCH 3/3] Clarify --- .../main/java/org/elasticsearch/server/cli/JvmErgonomics.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java index 3af0031df7c56..926d5727a1b4a 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/JvmErgonomics.java @@ -85,7 +85,7 @@ static boolean tuneG1GCInitiatingHeapOccupancyPercent(final Map * * @throws IllegalStateException if neither `-XX:+UseG1GC` nor `-XX:-UseG1GC` is in the final JVM options, or `-XX:+UseG1GC` is selected - * and {@code optionName} is missing. + * and {@code optionName} is not in the final JVM options. */ private static boolean usingG1GcWithoutCommandLineOriginOption(Map finalJvmOptions, String optionName) { return getRequiredOption(finalJvmOptions, "UseG1GC").getMandatoryValue().equals("true")