From b19678271f3b5506d0731096b2925530b97ed13c Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 9 Apr 2025 14:32:42 -0700 Subject: [PATCH 1/4] Ensure logs dir exists before using as working dir With the change to using the logs dir as the working dir of the Elasticsearch process we need to ensure the logs dir exists within the CLI instead of later during startup. relates #124966 --- .../elasticsearch/server/cli/ServerCli.java | 3 +-- .../server/cli/ServerProcessBuilder.java | 23 +++++++++++++------ .../server/cli/ServerProcessTests.java | 10 +++----- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerCli.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerCli.java index 8eae170667a84..5837f5fa8557e 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerCli.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerCli.java @@ -271,8 +271,7 @@ protected ServerProcess startServer(Terminal terminal, ProcessInfo processInfo, .withProcessInfo(processInfo) .withServerArgs(args) .withTempDir(tempDir) - .withJvmOptions(jvmOptions) - .withWorkingDir(args.logsDir()); + .withJvmOptions(jvmOptions); return serverProcessBuilder.start(); } diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java index 12a8bc00d4209..86349a915fb3e 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java @@ -10,6 +10,7 @@ package org.elasticsearch.server.cli; import org.elasticsearch.bootstrap.ServerArgs; +import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.ProcessInfo; import org.elasticsearch.cli.Terminal; import org.elasticsearch.cli.UserException; @@ -18,9 +19,11 @@ import org.elasticsearch.core.PathUtils; import org.elasticsearch.core.SuppressForbidden; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.OutputStream; import java.io.UncheckedIOException; +import java.nio.file.Files; import java.nio.file.Path; import java.util.HashMap; import java.util.List; @@ -44,7 +47,6 @@ public class ServerProcessBuilder { private ServerArgs serverArgs; private ProcessInfo processInfo; private List jvmOptions; - private Path workingDir; private Terminal terminal; // this allows mocking the process building by tests @@ -84,11 +86,6 @@ public ServerProcessBuilder withJvmOptions(List jvmOptions) { return this; } - public ServerProcessBuilder withWorkingDir(Path workingDir) { - this.workingDir = workingDir; - return this; - } - /** * Specifies the {@link Terminal} to use for reading input and writing output from/to the cli console */ @@ -138,9 +135,21 @@ private String getCommand() { * @throws UserException If the process failed during bootstrap */ public ServerProcess start() throws UserException { + ensureWorkingDirExists(); return start(ProcessBuilder::start); } + private void ensureWorkingDirExists() throws UserException { + Path workingDir = serverArgs.logsDir(); + try { + Files.createDirectories(workingDir); + } catch (FileNotFoundException e) { + throw new UserException(ExitCodes.CONFIG, "Logs dir [" + workingDir + "] exists but is not a directory", e); + } catch (IOException e) { + throw new UserException(ExitCodes.CONFIG, "Unable to create logs dir [" + workingDir + "]", e); + } + } + private static void checkRequiredArgument(Object argument, String argumentName) { if (argument == null) { throw new IllegalStateException( @@ -162,7 +171,7 @@ ServerProcess start(ProcessStarter processStarter) throws UserException { boolean success = false; try { - jvmProcess = createProcess(getCommand(), getJvmArgs(), jvmOptions, getEnvironment(), workingDir, processStarter); + jvmProcess = createProcess(getCommand(), getJvmArgs(), jvmOptions, getEnvironment(), serverArgs.logsDir(), processStarter); errorPump = new ErrorPumpThread(terminal, jvmProcess.getErrorStream()); errorPump.start(); sendArgs(serverArgs, jvmProcess.getOutputStream()); diff --git a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java index c9e233e22dd76..92bfae8498223 100644 --- a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java +++ b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java @@ -65,7 +65,6 @@ public class ServerProcessTests extends ESTestCase { protected final Map sysprops = new HashMap<>(); protected final Map envVars = new HashMap<>(); Path esHomeDir; - Path workingDir; Settings.Builder nodeSettings; ProcessValidator processValidator; MainMethod mainCallback; @@ -95,7 +94,6 @@ public void resetEnv() { sysprops.put("java.home", "javahome"); sysprops.put("es.path.home", esHomeDir.toString()); envVars.clear(); - workingDir = createTempDir(); nodeSettings = Settings.builder(); processValidator = null; mainCallback = null; @@ -231,8 +229,7 @@ ServerProcess startProcess(boolean daemonize, boolean quiet) throws Exception { .withProcessInfo(pinfo) .withServerArgs(createServerArgs(daemonize, quiet)) .withJvmOptions(List.of()) - .withTempDir(ServerProcessUtils.setupTempDir(pinfo)) - .withWorkingDir(workingDir); + .withTempDir(ServerProcessUtils.setupTempDir(pinfo)); return serverProcessBuilder.start(starter); } @@ -241,7 +238,7 @@ public void testProcessBuilder() throws Exception { assertThat(pb.redirectInput(), equalTo(ProcessBuilder.Redirect.PIPE)); assertThat(pb.redirectOutput(), equalTo(ProcessBuilder.Redirect.INHERIT)); assertThat(pb.redirectError(), equalTo(ProcessBuilder.Redirect.PIPE)); - assertThat(String.valueOf(pb.directory()), equalTo(workingDir.toString())); // leave default, which is working directory + assertThat(String.valueOf(pb.directory()), equalTo(esHomeDir.resolve("logs").toString())); }; mainCallback = (args, stdin, stderr, exitCode) -> { try (PrintStream err = new PrintStream(stderr, true, StandardCharsets.UTF_8)) { @@ -315,8 +312,7 @@ public void testCommandLineSysprops() throws Exception { .withProcessInfo(createProcessInfo()) .withServerArgs(createServerArgs(false, false)) .withJvmOptions(List.of("-Dfoo1=bar", "-Dfoo2=baz")) - .withTempDir(Path.of(".")) - .withWorkingDir(workingDir); + .withTempDir(Path.of(".")); serverProcessBuilder.start(starter).waitFor(); } From c019d288d88cf38bf6ea0f0db64e66ffb9ba9ce3 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 14 Apr 2025 15:25:37 -0700 Subject: [PATCH 2/4] add tests --- .../server/cli/ServerProcessBuilder.java | 6 ++-- .../server/cli/ServerProcessTests.java | 28 ++++++++++++++++++- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java index 86349a915fb3e..5c94faa26fb42 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.io.OutputStream; import java.io.UncheckedIOException; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; import java.util.HashMap; @@ -135,7 +136,6 @@ private String getCommand() { * @throws UserException If the process failed during bootstrap */ public ServerProcess start() throws UserException { - ensureWorkingDirExists(); return start(ProcessBuilder::start); } @@ -143,7 +143,7 @@ private void ensureWorkingDirExists() throws UserException { Path workingDir = serverArgs.logsDir(); try { Files.createDirectories(workingDir); - } catch (FileNotFoundException e) { + } catch (FileAlreadyExistsException e) { throw new UserException(ExitCodes.CONFIG, "Logs dir [" + workingDir + "] exists but is not a directory", e); } catch (IOException e) { throw new UserException(ExitCodes.CONFIG, "Unable to create logs dir [" + workingDir + "]", e); @@ -166,6 +166,8 @@ ServerProcess start(ProcessStarter processStarter) throws UserException { checkRequiredArgument(jvmOptions, "jvmOptions"); checkRequiredArgument(terminal, "terminal"); + ensureWorkingDirExists(); + Process jvmProcess = null; ErrorPumpThread errorPump; diff --git a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java index 92bfae8498223..52344b439a7a7 100644 --- a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java +++ b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java @@ -20,6 +20,7 @@ import org.elasticsearch.common.settings.SecureSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.IOUtils; +import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; import org.junit.AfterClass; import org.junit.Before; @@ -32,6 +33,7 @@ import java.io.PrintStream; import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -65,6 +67,7 @@ public class ServerProcessTests extends ESTestCase { protected final Map sysprops = new HashMap<>(); protected final Map envVars = new HashMap<>(); Path esHomeDir; + Path logsDir; Settings.Builder nodeSettings; ProcessValidator processValidator; MainMethod mainCallback; @@ -93,6 +96,7 @@ public void resetEnv() { sysprops.put("os.name", "Linux"); sysprops.put("java.home", "javahome"); sysprops.put("es.path.home", esHomeDir.toString()); + logsDir = esHomeDir.resolve("logs"); envVars.clear(); nodeSettings = Settings.builder(); processValidator = null; @@ -212,7 +216,7 @@ ServerArgs createServerArgs(boolean daemonize, boolean quiet) { secrets, nodeSettings.build(), esHomeDir.resolve("config"), - esHomeDir.resolve("logs") + logsDir ); } @@ -429,4 +433,26 @@ public void testProcessDies() throws Exception { int exitCode = server.waitFor(); assertThat(exitCode, equalTo(-9)); } + + public void testLogsDirIsFile() throws Exception { + Files.createFile(logsDir); + var e = expectThrows(UserException.class, this::runForeground); + assertThat(e.getMessage(), containsString("exists but is not a directory")); + } + + public void testLogsDirCreateParents() throws Exception { + Path testDir = createTempDir(); + logsDir = testDir.resolve("subdir/logs"); + processValidator = pb -> assertThat(pb.directory().toString(), equalTo(logsDir.toString())); + runForeground(); + } + + public void testLogsCreateFailure() throws Exception { + Path testDir = createTempDir(); + Path parentFile = testDir.resolve("exists"); + Files.createFile(parentFile); + logsDir = parentFile.resolve("logs"); + var e = expectThrows(UserException.class, this::runForeground); + assertThat(e.getMessage(), containsString("Unable to create logs dir")); + } } From 123cca61b3ef3d15f72e8c727638a9dc8236ba91 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Mon, 14 Apr 2025 22:34:12 +0000 Subject: [PATCH 3/4] [CI] Auto commit changes from spotless --- .../server/cli/ServerProcessBuilder.java | 1 - .../elasticsearch/server/cli/ServerProcessTests.java | 12 +----------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java index 5c94faa26fb42..375a12ea5cc7b 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcessBuilder.java @@ -19,7 +19,6 @@ import org.elasticsearch.core.PathUtils; import org.elasticsearch.core.SuppressForbidden; -import java.io.FileNotFoundException; import java.io.IOException; import java.io.OutputStream; import java.io.UncheckedIOException; diff --git a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java index 52344b439a7a7..0c5a670870890 100644 --- a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java +++ b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java @@ -20,7 +20,6 @@ import org.elasticsearch.common.settings.SecureSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.IOUtils; -import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; import org.junit.AfterClass; import org.junit.Before; @@ -33,7 +32,6 @@ import java.io.PrintStream; import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; -import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -209,15 +207,7 @@ ProcessInfo createProcessInfo() { } ServerArgs createServerArgs(boolean daemonize, boolean quiet) { - return new ServerArgs( - daemonize, - quiet, - null, - secrets, - nodeSettings.build(), - esHomeDir.resolve("config"), - logsDir - ); + return new ServerArgs(daemonize, quiet, null, secrets, nodeSettings.build(), esHomeDir.resolve("config"), logsDir); } ServerProcess startProcess(boolean daemonize, boolean quiet) throws Exception { From d4febc1a2d5bae930b61ec4603f65ebb76e9f3f2 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 15 Apr 2025 15:44:28 -0700 Subject: [PATCH 4/4] workaround forbidden --- .../java/org/elasticsearch/server/cli/ServerProcessTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java index 0c5a670870890..86bc336f0b4ba 100644 --- a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java +++ b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java @@ -433,7 +433,7 @@ public void testLogsDirIsFile() throws Exception { public void testLogsDirCreateParents() throws Exception { Path testDir = createTempDir(); logsDir = testDir.resolve("subdir/logs"); - processValidator = pb -> assertThat(pb.directory().toString(), equalTo(logsDir.toString())); + processValidator = pb -> assertThat(String.valueOf(pb.directory()), equalTo(logsDir.toString())); runForeground(); }