From afcf68322899c830c32fa3881cf42bd038bfdc7e Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 30 Sep 2016 02:18:54 +0200 Subject: [PATCH] Remove ignore system bootstrap checks Today we allow system bootstrap checks to be ignored with a setting. Yet, the system bootstrap checks are as vital to the health of a production node as the non-system checks (e.g., the original bootstrap check, the file descriptor check, is critical for reducing the chances of data loss from being too low). This commit removes the ability to ignore system bootstrap checks. Relates #20511 --- .../bootstrap/BootstrapCheck.java | 67 +---------- .../bootstrap/BootstrapSettings.java | 2 - .../common/settings/ClusterSettings.java | 1 - .../bootstrap/BootstrapCheckTests.java | 113 +++++------------- 4 files changed, 29 insertions(+), 154 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCheck.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCheck.java index 28f82308cbb..de80b487c7e 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCheck.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCheck.java @@ -66,7 +66,6 @@ final class BootstrapCheck { static void check(final Settings settings, final BoundTransportAddress boundTransportAddress) throws NodeValidationException { check( enforceLimits(boundTransportAddress), - BootstrapSettings.IGNORE_SYSTEM_BOOTSTRAP_CHECKS.get(settings), checks(settings), Node.NODE_NAME_SETTING.get(settings)); } @@ -77,18 +76,15 @@ final class BootstrapCheck { * * @param enforceLimits true if the checks should be enforced or * otherwise warned - * @param ignoreSystemChecks true if system checks should be enforced - * or otherwise warned * @param checks the checks to execute * @param nodeName the node name to be used as a logging prefix */ // visible for testing static void check( final boolean enforceLimits, - final boolean ignoreSystemChecks, final List checks, final String nodeName) throws NodeValidationException { - check(enforceLimits, ignoreSystemChecks, checks, Loggers.getLogger(BootstrapCheck.class, nodeName)); + check(enforceLimits, checks, Loggers.getLogger(BootstrapCheck.class, nodeName)); } /** @@ -97,14 +93,11 @@ final class BootstrapCheck { * * @param enforceLimits true if the checks should be enforced or * otherwise warned - * @param ignoreSystemChecks true if system checks should be enforced - * or otherwise warned * @param checks the checks to execute * @param logger the logger to */ static void check( final boolean enforceLimits, - final boolean ignoreSystemChecks, final List checks, final Logger logger) throws NodeValidationException { final List errors = new ArrayList<>(); @@ -113,13 +106,10 @@ final class BootstrapCheck { if (enforceLimits) { logger.info("bound or publishing to a non-loopback or non-link-local address, enforcing bootstrap checks"); } - if (enforceLimits && ignoreSystemChecks) { - logger.warn("enforcing bootstrap checks but ignoring system bootstrap checks, consider not ignoring system checks"); - } for (final Check check : checks) { if (check.check()) { - if ((!enforceLimits || (check.isSystemCheck() && ignoreSystemChecks)) && !check.alwaysEnforce()) { + if (!enforceLimits && !check.alwaysEnforce()) { ignoredErrors.add(check.errorMessage()); } else { errors.add(check.errorMessage()); @@ -201,14 +191,6 @@ final class BootstrapCheck { */ String errorMessage(); - /** - * test if the check is a system-level check - * - * @return true if the check is a system-level check as opposed - * to an Elasticsearch-level check - */ - boolean isSystemCheck(); - default boolean alwaysEnforce() { return false; } @@ -245,11 +227,6 @@ final class BootstrapCheck { return JvmInfo.jvmInfo().getConfiguredMaxHeapSize(); } - @Override - public final boolean isSystemCheck() { - return false; - } - } static class OsXFileDescriptorCheck extends FileDescriptorCheck { @@ -299,11 +276,6 @@ final class BootstrapCheck { return ProcessProbe.getInstance().getMaxFileDescriptorCount(); } - @Override - public final boolean isSystemCheck() { - return true; - } - } static class MlockallCheck implements Check { @@ -329,11 +301,6 @@ final class BootstrapCheck { return Natives.isMemoryLocked(); } - @Override - public final boolean isSystemCheck() { - return true; - } - } static class MaxNumberOfThreadsCheck implements Check { @@ -360,11 +327,6 @@ final class BootstrapCheck { return JNANatives.MAX_NUMBER_OF_THREADS; } - @Override - public final boolean isSystemCheck() { - return true; - } - } static class MaxSizeVirtualMemoryCheck implements Check { @@ -393,11 +355,6 @@ final class BootstrapCheck { return JNANatives.MAX_SIZE_VIRTUAL_MEMORY; } - @Override - public final boolean isSystemCheck() { - return true; - } - } static class MaxMapCountCheck implements Check { @@ -465,11 +422,6 @@ final class BootstrapCheck { return Long.parseLong(procSysVmMaxMapCount); } - @Override - public final boolean isSystemCheck() { - return true; - } - } static class ClientJvmCheck implements BootstrapCheck.Check { @@ -492,11 +444,6 @@ final class BootstrapCheck { getVmName()); } - @Override - public final boolean isSystemCheck() { - return false; - } - } /** @@ -524,11 +471,6 @@ final class BootstrapCheck { JvmInfo.jvmInfo().getVmName()); } - @Override - public boolean isSystemCheck() { - return false; - } - } abstract static class MightForkCheck implements BootstrapCheck.Check { @@ -546,11 +488,6 @@ final class BootstrapCheck { // visible for testing abstract boolean mightFork(); - @Override - public final boolean isSystemCheck() { - return false; - } - @Override public final boolean alwaysEnforce() { return true; diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapSettings.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapSettings.java index ad37916881b..e8015d83af3 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapSettings.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapSettings.java @@ -37,7 +37,5 @@ public final class BootstrapSettings { Setting.boolSetting("bootstrap.seccomp", true, Property.NodeScope); public static final Setting CTRLHANDLER_SETTING = Setting.boolSetting("bootstrap.ctrlhandler", true, Property.NodeScope); - public static final Setting IGNORE_SYSTEM_BOOTSTRAP_CHECKS = - Setting.boolSetting("bootstrap.ignore_system_bootstrap_checks", false, Property.NodeScope); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index c1841d11fbf..b5a0564174a 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -397,7 +397,6 @@ public final class ClusterSettings extends AbstractScopedSettings { BootstrapSettings.MEMORY_LOCK_SETTING, BootstrapSettings.SECCOMP_SETTING, BootstrapSettings.CTRLHANDLER_SETTING, - BootstrapSettings.IGNORE_SYSTEM_BOOTSTRAP_CHECKS, IndexingMemoryController.INDEX_BUFFER_SIZE_SETTING, IndexingMemoryController.MIN_INDEX_BUFFER_SIZE_SETTING, IndexingMemoryController.MAX_INDEX_BUFFER_SIZE_SETTING, diff --git a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapCheckTests.java b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapCheckTests.java index c038525fb0e..9813731017d 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapCheckTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapCheckTests.java @@ -70,18 +70,14 @@ public class BootstrapCheckTests extends ESTestCase { public void testNoLogMessageInNonProductionMode() throws NodeValidationException { final Logger logger = mock(Logger.class); - BootstrapCheck.check(false, randomBoolean(), Collections.emptyList(), logger); + BootstrapCheck.check(false, Collections.emptyList(), logger); verifyNoMoreInteractions(logger); } public void testLogMessageInProductionMode() throws NodeValidationException { final Logger logger = mock(Logger.class); - final boolean ignoreSystemChecks = randomBoolean(); - BootstrapCheck.check(true, ignoreSystemChecks, Collections.emptyList(), logger); + BootstrapCheck.check(true, Collections.emptyList(), logger); verify(logger).info("bound or publishing to a non-loopback or non-link-local address, enforcing bootstrap checks"); - if (ignoreSystemChecks) { - verify(logger).warn("enforcing bootstrap checks but ignoring system bootstrap checks, consider not ignoring system checks"); - } verifyNoMoreInteractions(logger); } @@ -139,11 +135,6 @@ public class BootstrapCheckTests extends ESTestCase { public String errorMessage() { return "first"; } - - @Override - public boolean isSystemCheck() { - return false; - } }, new BootstrapCheck.Check() { @Override @@ -155,16 +146,11 @@ public class BootstrapCheckTests extends ESTestCase { public String errorMessage() { return "second"; } - - @Override - public boolean isSystemCheck() { - return false; - } } ); final NodeValidationException e = - expectThrows(NodeValidationException.class, () -> BootstrapCheck.check(true, false, checks, "testExceptionAggregation")); + expectThrows(NodeValidationException.class, () -> BootstrapCheck.check(true, checks, "testExceptionAggregation")); assertThat(e, hasToString(allOf(containsString("bootstrap checks failed"), containsString("first"), containsString("second")))); final Throwable[] suppressed = e.getSuppressed(); assertThat(suppressed.length, equalTo(2)); @@ -195,7 +181,7 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testHeapSizeCheck")); + () -> BootstrapCheck.check(true, Collections.singletonList(check), "testHeapSizeCheck")); assertThat( e.getMessage(), containsString("initial heap size [" + initialHeapSize.get() + "] " + @@ -203,7 +189,7 @@ public class BootstrapCheckTests extends ESTestCase { initialHeapSize.set(maxHeapSize.get()); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testHeapSizeCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testHeapSizeCheck"); // nothing should happen if the initial heap size or the max // heap size is not available @@ -212,7 +198,7 @@ public class BootstrapCheckTests extends ESTestCase { } else { maxHeapSize.set(0); } - BootstrapCheck.check(true, false, Collections.singletonList(check), "testHeapSizeCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testHeapSizeCheck"); } public void testFileDescriptorLimits() throws NodeValidationException { @@ -238,17 +224,17 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows(NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testFileDescriptorLimits")); + () -> BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimits")); assertThat(e.getMessage(), containsString("max file descriptors")); maxFileDescriptorCount.set(randomIntBetween(limit + 1, Integer.MAX_VALUE)); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testFileDescriptorLimits"); + BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimits"); // nothing should happen if current file descriptor count is // not available maxFileDescriptorCount.set(-1); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testFileDescriptorLimits"); + BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimits"); } public void testFileDescriptorLimitsThrowsOnInvalidLimit() { @@ -293,7 +279,6 @@ public class BootstrapCheckTests extends ESTestCase { NodeValidationException.class, () -> BootstrapCheck.check( true, - false, Collections.singletonList(check), "testFileDescriptorLimitsThrowsOnInvalidLimit")); assertThat( @@ -301,7 +286,7 @@ public class BootstrapCheckTests extends ESTestCase { containsString("memory locking requested for elasticsearch process but memory is not locked")); } else { // nothing should happen - BootstrapCheck.check(true, false, Collections.singletonList(check), "testFileDescriptorLimitsThrowsOnInvalidLimit"); + BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimitsThrowsOnInvalidLimit"); } } } @@ -318,17 +303,17 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxNumberOfThreadsCheck")); + () -> BootstrapCheck.check(true, Collections.singletonList(check), "testMaxNumberOfThreadsCheck")); assertThat(e.getMessage(), containsString("max number of threads")); maxNumberOfThreads.set(randomIntBetween(limit + 1, Integer.MAX_VALUE)); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxNumberOfThreadsCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testMaxNumberOfThreadsCheck"); // nothing should happen if current max number of threads is // not available maxNumberOfThreads.set(-1); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxNumberOfThreadsCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testMaxNumberOfThreadsCheck"); } public void testMaxSizeVirtualMemory() throws NodeValidationException { @@ -349,17 +334,17 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxSizeVirtualMemory")); + () -> BootstrapCheck.check(true, Collections.singletonList(check), "testMaxSizeVirtualMemory")); assertThat(e.getMessage(), containsString("max size virtual memory")); maxSizeVirtualMemory.set(rlimInfinity); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxSizeVirtualMemory"); + BootstrapCheck.check(true, Collections.singletonList(check), "testMaxSizeVirtualMemory"); // nothing should happen if max size virtual memory is not // available maxSizeVirtualMemory.set(Long.MIN_VALUE); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxSizeVirtualMemory"); + BootstrapCheck.check(true, Collections.singletonList(check), "testMaxSizeVirtualMemory"); } public void testMaxMapCountCheck() throws NodeValidationException { @@ -374,17 +359,17 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxMapCountCheck")); + () -> BootstrapCheck.check(true, Collections.singletonList(check), "testMaxMapCountCheck")); assertThat(e.getMessage(), containsString("max virtual memory areas vm.max_map_count")); maxMapCount.set(randomIntBetween(limit + 1, Integer.MAX_VALUE)); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxMapCountCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testMaxMapCountCheck"); // nothing should happen if current vm.max_map_count is not // available maxMapCount.set(-1); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testMaxMapCountCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testMaxMapCountCheck"); } public void testClientJvmCheck() throws NodeValidationException { @@ -398,14 +383,14 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testClientJvmCheck")); + () -> BootstrapCheck.check(true, Collections.singletonList(check), "testClientJvmCheck")); assertThat( e.getMessage(), containsString("JVM is using the client VM [Java HotSpot(TM) 32-Bit Client VM] " + "but should be using a server VM for the best performance")); vmName.set("Java HotSpot(TM) 32-Bit Server VM"); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testClientJvmCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testClientJvmCheck"); } public void testUseSerialGCCheck() throws NodeValidationException { @@ -419,14 +404,14 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testUseSerialGCCheck")); + () -> BootstrapCheck.check(true, Collections.singletonList(check), "testUseSerialGCCheck")); assertThat( e.getMessage(), containsString("JVM is using the serial collector but should not be for the best performance; " + "" + "either it's the default for the VM [" + JvmInfo.jvmInfo().getVmName() +"] or -XX:+UseSerialGC was explicitly specified")); useSerialGC.set("false"); - BootstrapCheck.check(true, false, Collections.singletonList(check), "testUseSerialGCCheck"); + BootstrapCheck.check(true, Collections.singletonList(check), "testUseSerialGCCheck"); } public void testMightForkCheck() throws NodeValidationException { @@ -530,13 +515,13 @@ public class BootstrapCheckTests extends ESTestCase { } else { enableMightFork.run(); } - BootstrapCheck.check(true, randomBoolean(), Collections.singletonList(check), methodName); + BootstrapCheck.check(true, Collections.singletonList(check), methodName); // if seccomp is enabled, but we will not fork, nothing should // happen isSeccompInstalled.set(true); disableMightFork.run(); - BootstrapCheck.check(true, randomBoolean(), Collections.singletonList(check), methodName); + BootstrapCheck.check(true, Collections.singletonList(check), methodName); // if seccomp is enabled, and we might fork, the check should // be enforced, regardless of bootstrap checks being enabled or @@ -546,49 +531,10 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException e = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(randomBoolean(), randomBoolean(), Collections.singletonList(check), methodName)); + () -> BootstrapCheck.check(randomBoolean(), Collections.singletonList(check), methodName)); consumer.accept(e); } - public void testIgnoringSystemChecks() throws NodeValidationException { - final BootstrapCheck.Check check = new BootstrapCheck.Check() { - @Override - public boolean check() { - return true; - } - - @Override - public String errorMessage() { - return "error"; - } - - @Override - public boolean isSystemCheck() { - return true; - } - }; - - final NodeValidationException notIgnored = expectThrows( - NodeValidationException.class, - () -> BootstrapCheck.check(true, false, Collections.singletonList(check), "testIgnoringSystemChecks")); - assertThat(notIgnored, hasToString(containsString("error"))); - - final Logger logger = mock(Logger.class); - - // nothing should happen if we ignore system checks - BootstrapCheck.check(true, true, Collections.singletonList(check), logger); - verify(logger).info("bound or publishing to a non-loopback or non-link-local address, enforcing bootstrap checks"); - verify(logger).warn("enforcing bootstrap checks but ignoring system bootstrap checks, consider not ignoring system checks"); - verify(logger).warn("error"); - verifyNoMoreInteractions(logger); - reset(logger); - - // nothing should happen if we ignore all checks - BootstrapCheck.check(false, randomBoolean(), Collections.singletonList(check), logger); - verify(logger).warn("error"); - verifyNoMoreInteractions(logger); - } - public void testAlwaysEnforcedChecks() { final BootstrapCheck.Check check = new BootstrapCheck.Check() { @Override @@ -601,11 +547,6 @@ public class BootstrapCheckTests extends ESTestCase { return "error"; } - @Override - public boolean isSystemCheck() { - return randomBoolean(); - } - @Override public boolean alwaysEnforce() { return true; @@ -614,7 +555,7 @@ public class BootstrapCheckTests extends ESTestCase { final NodeValidationException alwaysEnforced = expectThrows( NodeValidationException.class, - () -> BootstrapCheck.check(randomBoolean(), randomBoolean(), Collections.singletonList(check), "testAlwaysEnforcedChecks")); + () -> BootstrapCheck.check(randomBoolean(), Collections.singletonList(check), "testAlwaysEnforcedChecks")); assertThat(alwaysEnforced, hasToString(containsString("error"))); }