From 49ea53bb3b1a2c38b275c306fb71898b3c8f08b1 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 27 Sep 2019 12:04:02 +0100 Subject: [PATCH 1/9] Don't close stderr, print logs dir on fatal err Closes #46900. When running ES with --quiet, if ES then exits abnormally, a user has to go hunting in the logs for the error. Instead, never close System.err, and print more information to it if ES encounters a fatal error e.g. config validation, or some fatal runtime exception. --- .../java/org/elasticsearch/cli/Terminal.java | 4 +++ .../elasticsearch/bootstrap/Bootstrap.java | 9 ----- .../bootstrap/Elasticsearch.java | 6 ++++ ...ElasticsearchUncaughtExceptionHandler.java | 34 ++++++++++++------- .../bootstrap/StartupException.java | 6 +++- 5 files changed, 37 insertions(+), 22 deletions(-) diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java b/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java index 718b4796c0209..c62264fd4b30e 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java @@ -151,6 +151,10 @@ public final boolean promptYesNo(String prompt, boolean defaultYes) { } } + public void flush() { + this.getErrorWriter().flush(); + } + private static class ConsoleTerminal extends Terminal { private static final Console CONSOLE = System.console(); diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index fd34c48fa8ebf..c4f71923c7808 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -346,10 +346,6 @@ static void init( } INSTANCE.start(); - - if (closeStandardStreams) { - closeSysError(); - } } catch (NodeValidationException | RuntimeException e) { // disable console logging, so user does not see the exception twice (jvm will show it already) final Logger rootLogger = LogManager.getRootLogger(); @@ -397,11 +393,6 @@ private static void closeSystOut() { System.out.close(); } - @SuppressForbidden(reason = "System#err") - private static void closeSysError() { - System.err.close(); - } - private static void checkLucene() { if (Version.CURRENT.luceneVersion.equals(org.apache.lucene.util.Version.LATEST) == false) { throw new AssertionError("Lucene version mismatch this version of Elasticsearch requires lucene version [" diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java index 26a1aa30ada17..8ee81d7bc6794 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java @@ -91,6 +91,12 @@ public void checkPermission(Permission perm) { final Elasticsearch elasticsearch = new Elasticsearch(); int status = main(args, elasticsearch, Terminal.DEFAULT); if (status != ExitCodes.OK) { + Terminal.DEFAULT.errorPrintln("ERROR: Elasticsearch did not exit normally - check the logs at " + + System.getProperty("es.logs.base_path") + + System.getProperty("file.separator") + + System.getProperty("es.logs.cluster_name") + + ".log" + ); exit(status); } } diff --git a/server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchUncaughtExceptionHandler.java b/server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchUncaughtExceptionHandler.java index 1c3c0ccf6b61f..d2d5ea1ef6ea8 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchUncaughtExceptionHandler.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchUncaughtExceptionHandler.java @@ -21,7 +21,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.message.ParameterizedMessage; +import org.elasticsearch.cli.Terminal; import org.elasticsearch.common.SuppressForbidden; import java.io.IOError; @@ -32,29 +32,29 @@ class ElasticsearchUncaughtExceptionHandler implements Thread.UncaughtExceptionH private static final Logger logger = LogManager.getLogger(ElasticsearchUncaughtExceptionHandler.class); @Override - public void uncaughtException(Thread t, Throwable e) { - if (isFatalUncaught(e)) { + public void uncaughtException(Thread thread, Throwable t) { + if (isFatalUncaught(t)) { try { - onFatalUncaught(t.getName(), e); + onFatalUncaught(thread.getName(), t); } finally { // we use specific error codes in case the above notification failed, at least we // will have some indication of the error bringing us down - if (e instanceof InternalError) { + if (t instanceof InternalError) { halt(128); - } else if (e instanceof OutOfMemoryError) { + } else if (t instanceof OutOfMemoryError) { halt(127); - } else if (e instanceof StackOverflowError) { + } else if (t instanceof StackOverflowError) { halt(126); - } else if (e instanceof UnknownError) { + } else if (t instanceof UnknownError) { halt(125); - } else if (e instanceof IOError) { + } else if (t instanceof IOError) { halt(124); } else { halt(1); } } } else { - onNonFatalUncaught(t.getName(), e); + onNonFatalUncaught(thread.getName(), t); } } @@ -63,11 +63,21 @@ static boolean isFatalUncaught(Throwable e) { } void onFatalUncaught(final String threadName, final Throwable t) { - logger.error(() -> new ParameterizedMessage("fatal error in thread [{}], exiting", threadName), t); + final String message = "fatal error in thread [" + threadName + "], exiting"; + logger.error(message, t); + Terminal.DEFAULT.errorPrintln(message); + t.printStackTrace(Terminal.DEFAULT.getErrorWriter()); + // Without a final flush, the stacktrace may not be shown before ES exits + Terminal.DEFAULT.flush(); } void onNonFatalUncaught(final String threadName, final Throwable t) { - logger.warn(() -> new ParameterizedMessage("uncaught exception in thread [{}]", threadName), t); + final String message = "uncaught exception in thread [" + threadName + "]"; + logger.error(message, t); + Terminal.DEFAULT.errorPrintln(message); + t.printStackTrace(Terminal.DEFAULT.getErrorWriter()); + // Without a final flush, the stacktrace may not be shown if ES goes on to exit + Terminal.DEFAULT.flush(); } void halt(int status) { diff --git a/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java b/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java index 59629eb5b3b19..e744acab43fbc 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java @@ -111,7 +111,11 @@ private void printStackTrace(Consumer consumer) { // if its a guice exception, the whole thing really will not be in the log, its megabytes. // refer to the hack in bootstrap, where we don't log it if (originalCause instanceof CreationException == false) { - consumer.accept("Refer to the log for complete error details."); + final String logPath = System.getProperty("es.logs.base_path") + + System.getProperty("file.separator") + + System.getProperty("es.logs.cluster_name") + + ".log"; + consumer.accept("For complete error details, refer to the log at " + logPath); } } From f3973b33e774f705e9f025f89fbf54fa51a0af0f Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 10 Oct 2019 14:13:34 +0100 Subject: [PATCH 2/9] Close stderr when daemonizing The previous change never closed stderr, either when `--quiet` or when `-d` is passed. However, this broken the BATS tests, which expected to be able to quickly consume all output when passing `-d`. Thinking about it some more, it's reasonable (if perhaps still debatable) to leave stderr open under `--quiet`, which fixes the systemd case, but close stderr under `-d`. --- .../org/elasticsearch/bootstrap/Bootstrap.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index c4f71923c7808..f59f082596d7b 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -346,6 +346,15 @@ static void init( } INSTANCE.start(); + + // We don't close stderr if `--quiet` is passed, because that + // hides fatal startup errors. For example, if Elasticsearch is + // running via systemd, the init script only specifies + // `--quiet`, not `-d`, so we want users to be able to see + // startup errors via journalctl. + if (foreground == false) { + closeSysError(); + } } catch (NodeValidationException | RuntimeException e) { // disable console logging, so user does not see the exception twice (jvm will show it already) final Logger rootLogger = LogManager.getRootLogger(); @@ -393,6 +402,11 @@ private static void closeSystOut() { System.out.close(); } + @SuppressForbidden(reason = "System#err") + private static void closeSysError() { + System.err.close(); + } + private static void checkLucene() { if (Version.CURRENT.luceneVersion.equals(org.apache.lucene.util.Version.LATEST) == false) { throw new AssertionError("Lucene version mismatch this version of Elasticsearch requires lucene version [" From fad094815469ecfad85b287219b33f1f86893d96 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 23 Oct 2019 12:01:30 +0100 Subject: [PATCH 3/9] Guard against logging not being configured yet There's no point telling the user to go and check the logs if we haven't configured logging yet. --- .../elasticsearch/bootstrap/Elasticsearch.java | 17 +++++++++++------ .../bootstrap/StartupException.java | 16 +++++++++++----- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java index 8ee81d7bc6794..48bbd6d9c8c91 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java @@ -91,12 +91,17 @@ public void checkPermission(Permission perm) { final Elasticsearch elasticsearch = new Elasticsearch(); int status = main(args, elasticsearch, Terminal.DEFAULT); if (status != ExitCodes.OK) { - Terminal.DEFAULT.errorPrintln("ERROR: Elasticsearch did not exit normally - check the logs at " + - System.getProperty("es.logs.base_path") + - System.getProperty("file.separator") + - System.getProperty("es.logs.cluster_name") + - ".log" - ); + final String basePath = System.getProperty("es.logs.base_path"); + // It's possible to fail before logging has been configured, in which case there's no point + // suggested that the user look in the log file. + if (basePath != null) { + Terminal.DEFAULT.errorPrintln( + "ERROR: Elasticsearch did not exit normally - check the logs at " + + basePath + + System.getProperty("file.separator") + + System.getProperty("es.logs.cluster_name") + ".log" + ); + } exit(status); } } diff --git a/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java b/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java index e744acab43fbc..f9349361a4444 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/StartupException.java @@ -111,11 +111,17 @@ private void printStackTrace(Consumer consumer) { // if its a guice exception, the whole thing really will not be in the log, its megabytes. // refer to the hack in bootstrap, where we don't log it if (originalCause instanceof CreationException == false) { - final String logPath = System.getProperty("es.logs.base_path") + - System.getProperty("file.separator") + - System.getProperty("es.logs.cluster_name") + - ".log"; - consumer.accept("For complete error details, refer to the log at " + logPath); + final String basePath = System.getProperty("es.logs.base_path"); + // It's possible to fail before logging has been configured, in which case there's no point + // suggested that the user look in the log file. + if (basePath != null) { + final String logPath = System.getProperty("es.logs.base_path") + + System.getProperty("file.separator") + + System.getProperty("es.logs.cluster_name") + + ".log"; + + consumer.accept("For complete error details, refer to the log at " + logPath); + } } } From 4091c5594db2b756d86074605a0eb38ebbbb379c Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 31 Oct 2019 15:46:04 +0000 Subject: [PATCH 4/9] Address review feedback --- libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java | 1 + .../main/java/org/elasticsearch/bootstrap/Elasticsearch.java | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java b/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java index c62264fd4b30e..74af7e2e3102f 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java @@ -152,6 +152,7 @@ public final boolean promptYesNo(String prompt, boolean defaultYes) { } public void flush() { + this.getWriter().flush(); this.getErrorWriter().flush(); } diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java index 48bbd6d9c8c91..3513a967763f5 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java @@ -93,7 +93,7 @@ public void checkPermission(Permission perm) { if (status != ExitCodes.OK) { final String basePath = System.getProperty("es.logs.base_path"); // It's possible to fail before logging has been configured, in which case there's no point - // suggested that the user look in the log file. + // suggesting that the user look in the log file. if (basePath != null) { Terminal.DEFAULT.errorPrintln( "ERROR: Elasticsearch did not exit normally - check the logs at " From ca99dc374e60e29d6ff31f0571a75a54e50ed8bb Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 13 Nov 2019 10:58:14 +0000 Subject: [PATCH 5/9] Add test to check that errors are logged to stderr Refactor PackageTests so that the logic for preparing a custom config is extracted to a helper method that takes a closure. Add a test that configures two GC types, and check that the bootstrap checks fail. Also some other misc cleanups. --- .../packaging/test/PackageTests.java | 113 +++++++++++------- .../packaging/util/Packages.java | 34 +++++- 2 files changed, 98 insertions(+), 49 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java index 386603dcce538..33e2b88ac9c24 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java @@ -23,7 +23,6 @@ import org.apache.http.client.fluent.Request; import org.elasticsearch.packaging.util.FileUtils; import org.elasticsearch.packaging.util.Shell.Result; -import org.hamcrest.CoreMatchers; import org.junit.BeforeClass; import java.nio.charset.StandardCharsets; @@ -47,10 +46,12 @@ import static org.elasticsearch.packaging.util.Packages.SYSTEMD_SERVICE; import static org.elasticsearch.packaging.util.Packages.assertInstalled; import static org.elasticsearch.packaging.util.Packages.assertRemoved; +import static org.elasticsearch.packaging.util.Packages.clearJournal; import static org.elasticsearch.packaging.util.Packages.installPackage; import static org.elasticsearch.packaging.util.Packages.remove; import static org.elasticsearch.packaging.util.Packages.restartElasticsearch; import static org.elasticsearch.packaging.util.Packages.startElasticsearch; +import static org.elasticsearch.packaging.util.Packages.startElasticsearchIgnoringFailure; import static org.elasticsearch.packaging.util.Packages.stopElasticsearch; import static org.elasticsearch.packaging.util.Packages.verifyPackageInstallation; import static org.elasticsearch.packaging.util.Platforms.getOsRelease; @@ -60,7 +61,7 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.isEmptyString; +import static org.hamcrest.Matchers.emptyString; import static org.hamcrest.core.Is.is; import static org.junit.Assume.assumeThat; import static org.junit.Assume.assumeTrue; @@ -79,8 +80,8 @@ public void test10InstallPackage() throws Exception { verifyPackageInstallation(installation, distribution(), sh); } - public void test20PluginsCommandWhenNoPlugins() throws Exception { - assertThat(sh.run(installation.bin("elasticsearch-plugin") + " list").stdout, isEmptyString()); + public void test20PluginsCommandWhenNoPlugins() { + assertThat(sh.run(installation.bin("elasticsearch-plugin") + " list").stdout, is(emptyString())); } public void test30DaemonIsNotEnabledOnRestart() { @@ -95,7 +96,7 @@ public void test31InstallDoesNotStartServer() { assertThat(sh.run("ps aux").stdout, not(containsString("org.elasticsearch.bootstrap.Elasticsearch"))); } - public void assertRunsWithJavaHome() throws Exception { + private void assertRunsWithJavaHome() throws Exception { byte[] originalEnvFile = Files.readAllBytes(installation.envFile); try { Files.write(installation.envFile, ("JAVA_HOME=" + systemJavaHome + "\n").getBytes(StandardCharsets.UTF_8), @@ -286,53 +287,17 @@ public void test80DeletePID_DIRandRestart() throws Exception { } public void test81CustomPathConfAndJvmOptions() throws Exception { - assumeTrue(isSystemd()); - - assertPathsExist(installation.envFile); - - stopElasticsearch(sh); - - // The custom config directory is not under /tmp or /var/tmp because - // systemd's private temp directory functionally means different - // processes can have different views of what's in these directories - String randomName = RandomStrings.randomAsciiAlphanumOfLength(getRandom(), 10); - sh.run("mkdir /etc/"+randomName); - final Path tempConf = Paths.get("/etc/"+randomName); - - try { - mkdir(tempConf); - cp(installation.config("elasticsearch.yml"), tempConf.resolve("elasticsearch.yml")); - cp(installation.config("log4j2.properties"), tempConf.resolve("log4j2.properties")); - - // we have to disable Log4j from using JMX lest it will hit a security - // manager exception before we have configured logging; this will fail - // startup since we detect usages of logging before it is configured - final String jvmOptions = - "-Xms512m\n" + - "-Xmx512m\n" + - "-Dlog4j2.disable.jmx=true\n"; - append(tempConf.resolve("jvm.options"), jvmOptions); - - sh.runIgnoreExitCode("chown -R elasticsearch:elasticsearch " + tempConf); - - cp(installation.envFile, tempConf.resolve("elasticsearch.bk"));//backup - append(installation.envFile, "ES_PATH_CONF=" + tempConf + "\n"); + withCustomConfig(() -> { append(installation.envFile, "ES_JAVA_OPTS=-XX:-UseCompressedOops"); startElasticsearch(sh, installation); final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_nodes")); - assertThat(nodesResponse, CoreMatchers.containsString("\"heap_init_in_bytes\":536870912")); - assertThat(nodesResponse, CoreMatchers.containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); + assertThat(nodesResponse, containsString("\"heap_init_in_bytes\":536870912")); + assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); stopElasticsearch(sh); - - } finally { - rm(installation.envFile); - cp(tempConf.resolve("elasticsearch.bk"), installation.envFile); - rm(tempConf); - cleanup(); - } + }); } public void test82SystemdMask() throws Exception { @@ -374,4 +339,62 @@ public void test83serviceFileSetsLimits() throws Exception { stopElasticsearch(sh); } + + public void test90DoNotCloseStderrWhenQuiet() throws Exception { + withCustomConfig(() -> { + // Create a bootstrap problem by configuring an extra GC type + append(installation.envFile, "ES_JAVA_OPTS=\"-XX:+UseSerialGC -XX:+UseG1GC\"\n"); + + // Make sure we don't pick up the journal entries for previous ES instances. + clearJournal(sh); + startElasticsearchIgnoringFailure(sh); + + final Result logs = sh.run("journalctl -u elasticsearch.service"); + + assertThat(logs.stdout, containsString("Multiple garbage collectors selected")); + }); + } + + private void withCustomConfig(CheckedRunnable runnable) throws Exception { + assumeTrue(isSystemd()); + + assertPathsExist(installation.envFile); + + stopElasticsearch(sh); + + // The custom config directory is not under /tmp or /var/tmp because + // systemd's private temp directory functionally means different + // processes can have different views of what's in these directories + String randomName = RandomStrings.randomAsciiAlphanumOfLength(getRandom(), 10); + sh.run("mkdir /etc/" + randomName); + final Path tempConf = Paths.get("/etc/" + randomName); + + try { + mkdir(tempConf); + cp(installation.config("elasticsearch.yml"), tempConf.resolve("elasticsearch.yml")); + cp(installation.config("log4j2.properties"), tempConf.resolve("log4j2.properties")); + + // we have to disable Log4j from using JMX lest it will hit a security + // manager exception before we have configured logging; this will fail + // startup since we detect usages of logging before it is configured + final String jvmOptions = "-Xms512m\n-Xmx512m\n-Dlog4j2.disable.jmx=true\n"; + append(tempConf.resolve("jvm.options"), jvmOptions); + + sh.runIgnoreExitCode("chown -R elasticsearch:elasticsearch " + tempConf); + + cp(installation.envFile, tempConf.resolve("elasticsearch.bk"));// backup + append(installation.envFile, "ES_PATH_CONF=" + tempConf + "\n"); + + runnable.run(); + } finally { + rm(installation.envFile); + cp(tempConf.resolve("elasticsearch.bk"), installation.envFile); + rm(tempConf); + cleanup(); + } + } + + private interface CheckedRunnable { + void run() throws Exception; + } } diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java index c9fca10b5c8cf..38080740ecc2e 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Packages.java @@ -52,7 +52,7 @@ public class Packages { - protected static final Logger logger = LogManager.getLogger(Packages.class); + private static final Logger logger = LogManager.getLogger(Packages.class); public static final Path SYSVINIT_SCRIPT = Paths.get("/etc/init.d/elasticsearch"); public static final Path SYSTEMD_SERVICE = Paths.get("/usr/lib/systemd/system/elasticsearch.service"); @@ -114,7 +114,7 @@ public static Installation installPackage(Distribution distribution) throws IOEx return installation; } - public static Result runInstallCommand(Distribution distribution, Shell sh) { + private static Result runInstallCommand(Distribution distribution, Shell sh) { final Path distributionFile = distribution.path; if (Platforms.isRPM()) { @@ -280,7 +280,33 @@ public static void startElasticsearch(Shell sh, Installation installation) throw assertElasticsearchStarted(sh, installation); } - public static void assertElasticsearchStarted(Shell sh, Installation installation) throws IOException { + /** + * Starts Elasticsearch, without checking that startup is successful. To also check + * that Elasticsearch has started, call {@link #startElasticsearch(Shell, Installation)}. + */ + public static void startElasticsearchIgnoringFailure(Shell sh) { + if (isSystemd()) { + sh.runIgnoreExitCode("systemctl daemon-reload"); + sh.runIgnoreExitCode("systemctl enable elasticsearch.service"); + sh.runIgnoreExitCode("systemctl is-enabled elasticsearch.service"); + sh.runIgnoreExitCode("systemctl start elasticsearch.service"); + } else { + sh.runIgnoreExitCode("service elasticsearch start"); + } + } + + /** + * Clears the systemd journal. This is intended to clear the journalctl output + * before a test that checks the journal output. + */ + public static void clearJournal(Shell sh) { + if (isSystemd()) { + sh.run("rm -rf /run/log/journal/"); + sh.run("systemctl restart systemd-journald"); + } + } + + private static void assertElasticsearchStarted(Shell sh, Installation installation) throws IOException { waitForElasticsearch(installation); if (isSystemd()) { @@ -291,7 +317,7 @@ public static void assertElasticsearchStarted(Shell sh, Installation installatio } } - public static void stopElasticsearch(Shell sh) throws IOException { + public static void stopElasticsearch(Shell sh) { if (isSystemd()) { sh.run("systemctl stop elasticsearch.service"); } else { From 1a255ae43dec768107da7174ef1a1d793f6263d9 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 13 Nov 2019 13:06:12 +0000 Subject: [PATCH 6/9] Remove unnecessary interfaces --- .../org/elasticsearch/packaging/test/PackageTests.java | 5 +---- .../test/java/org/elasticsearch/packaging/util/Docker.java | 7 ++----- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java index 33e2b88ac9c24..5774b9628da43 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java @@ -21,6 +21,7 @@ import com.carrotsearch.randomizedtesting.generators.RandomStrings; import org.apache.http.client.fluent.Request; +import org.elasticsearch.common.CheckedRunnable; import org.elasticsearch.packaging.util.FileUtils; import org.elasticsearch.packaging.util.Shell.Result; import org.junit.BeforeClass; @@ -393,8 +394,4 @@ private void withCustomConfig(CheckedRunnable runnable) throws Exception { cleanup(); } } - - private interface CheckedRunnable { - void run() throws Exception; - } } diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index 3920fc9bb3892..a1a4a4328093a 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -21,6 +21,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.elasticsearch.common.CheckedRunnable; import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermission; @@ -409,7 +410,7 @@ public static void waitForElasticsearch(String status, String index, Installatio withLogging(() -> ServerUtils.waitForElasticsearch(status, index, installation, username, password)); } - private static void withLogging(ThrowingRunnable r) throws Exception { + private static void withLogging(CheckedRunnable r) throws Exception { try { r.run(); } catch (Exception e) { @@ -418,8 +419,4 @@ private static void withLogging(ThrowingRunnable r) throws Exception { throw e; } } - - private interface ThrowingRunnable { - void run() throws Exception; - } } From 52bb8293e24bf13da8040663f4dd447b78fb19f6 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 13 Nov 2019 13:52:49 +0000 Subject: [PATCH 7/9] Fix compile problem --- .../java/org/elasticsearch/packaging/test/PackageTests.java | 2 +- .../src/test/java/org/elasticsearch/packaging/util/Docker.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java index 5774b9628da43..a7aee80b3b835 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java @@ -356,7 +356,7 @@ public void test90DoNotCloseStderrWhenQuiet() throws Exception { }); } - private void withCustomConfig(CheckedRunnable runnable) throws Exception { + private void withCustomConfig(CheckedRunnable runnable) throws Exception { assumeTrue(isSystemd()); assertPathsExist(installation.envFile); diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index a1a4a4328093a..8489ef42ea83c 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -410,7 +410,7 @@ public static void waitForElasticsearch(String status, String index, Installatio withLogging(() -> ServerUtils.waitForElasticsearch(status, index, installation, username, password)); } - private static void withLogging(CheckedRunnable r) throws Exception { + private static void withLogging(CheckedRunnable r) throws Exception { try { r.run(); } catch (Exception e) { From 5fabb63c73eb74e57bd1b5d99dd2d69a232bc296 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 14 Nov 2019 10:13:24 +0000 Subject: [PATCH 8/9] Rework new packaging test The new test for checking stderr output was actually checking what Java emits when multiple GC strategies are configured. Now the test configures a broken YAML config, and check that ES reports this to stderr. --- .../packaging/test/PackageTests.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java index a7aee80b3b835..8d28128fa9199 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java @@ -31,6 +31,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardOpenOption; +import java.util.function.Consumer; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -288,7 +289,7 @@ public void test80DeletePID_DIRandRestart() throws Exception { } public void test81CustomPathConfAndJvmOptions() throws Exception { - withCustomConfig(() -> { + withCustomConfig(tempConf -> { append(installation.envFile, "ES_JAVA_OPTS=-XX:-UseCompressedOops"); startElasticsearch(sh, installation); @@ -342,9 +343,9 @@ public void test83serviceFileSetsLimits() throws Exception { } public void test90DoNotCloseStderrWhenQuiet() throws Exception { - withCustomConfig(() -> { - // Create a bootstrap problem by configuring an extra GC type - append(installation.envFile, "ES_JAVA_OPTS=\"-XX:+UseSerialGC -XX:+UseG1GC\"\n"); + withCustomConfig(tempConf -> { + // Create a startup problem by adding an invalid YAML line to the config + append(tempConf.resolve("elasticsearch.yml"), "discovery.zen.ping.unicast.hosts:15172.30.5.3416172.30.5.35, 172.30.5.17]\n"); // Make sure we don't pick up the journal entries for previous ES instances. clearJournal(sh); @@ -352,11 +353,16 @@ public void test90DoNotCloseStderrWhenQuiet() throws Exception { final Result logs = sh.run("journalctl -u elasticsearch.service"); - assertThat(logs.stdout, containsString("Multiple garbage collectors selected")); + assertThat(logs.stdout, containsString("Failed to load settings from [elasticsearch.yml]")); }); } - private void withCustomConfig(CheckedRunnable runnable) throws Exception { + @FunctionalInterface + private interface CustomConfigConsumer { + void accept(Path path) throws Exception; + } + + private void withCustomConfig(CustomConfigConsumer pathConsumer) throws Exception { assumeTrue(isSystemd()); assertPathsExist(installation.envFile); @@ -386,7 +392,7 @@ private void withCustomConfig(CheckedRunnable runnable) cp(installation.envFile, tempConf.resolve("elasticsearch.bk"));// backup append(installation.envFile, "ES_PATH_CONF=" + tempConf + "\n"); - runnable.run(); + pathConsumer.accept(tempConf); } finally { rm(installation.envFile); cp(tempConf.resolve("elasticsearch.bk"), installation.envFile); From 8850f1468642bd6d9e896cdc3d0e4d72a5a8645d Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 14 Nov 2019 11:02:38 +0000 Subject: [PATCH 9/9] Checkstyle --- .../java/org/elasticsearch/packaging/test/PackageTests.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java index 8d28128fa9199..155048be8a630 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java @@ -21,7 +21,6 @@ import com.carrotsearch.randomizedtesting.generators.RandomStrings; import org.apache.http.client.fluent.Request; -import org.elasticsearch.common.CheckedRunnable; import org.elasticsearch.packaging.util.FileUtils; import org.elasticsearch.packaging.util.Shell.Result; import org.junit.BeforeClass; @@ -31,7 +30,6 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardOpenOption; -import java.util.function.Consumer; import java.util.regex.Matcher; import java.util.regex.Pattern;