Move pidfile handling to server cli (#86934)

Now that the server cli is in java, we can do more system level things
inside it. This commit moves validating and writing the pidfile into the
server cli. One benefit is we get validation of directory/file problems
up front before even trying to start the ES process.

distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerCli.java

@@ -30,6 +30,7 @@
 import org.elasticsearch.monitor.jvm.JvmInfo;
 
 import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.Locale;
@@ -163,13 +164,26 @@ private Environment autoConfigureSecurity(
             env = createEnv(options, processInfo);
         }
         return env;
+    }
 
+    private void validatePidFile(Path pidFile) throws UserException {
+        Path parent = pidFile.getParent();
+        if (parent != null && Files.exists(parent) && Files.isDirectory(parent) == false) {
+            throw new UserException(ExitCodes.USAGE, "pid file parent [" + parent + "] exists but is not a directory");
+        }
+        if (Files.exists(pidFile) && Files.isRegularFile(pidFile) == false) {
+            throw new UserException(ExitCodes.USAGE, pidFile + " exists but is not a regular file");
+        }
     }
 
-    private ServerArgs createArgs(OptionSet options, Environment env, SecureString keystorePassword) {
+    private ServerArgs createArgs(OptionSet options, Environment env, SecureString keystorePassword) throws UserException {
         boolean daemonize = options.has(daemonizeOption);
         boolean quiet = options.has(quietOption);
-        Path pidFile = options.valueOf(pidfileOption);
+        Path pidFile = null;
+        if (options.has(pidfileOption)) {
+            pidFile = options.valueOf(pidfileOption);
+            validatePidFile(pidFile);
+        }
         return new ServerArgs(daemonize, quiet, pidFile, keystorePassword, env.settings(), env.configFile());
     }
 

distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcess.java

@@ -128,6 +128,13 @@ static ServerProcess start(
         return new ServerProcess(jvmProcess, errorPump);
     }
 
+    /**
+     * Return the process id of the server.
+     */
+    public long pid() {
+        return jvmProcess.pid();
+    }
+
     /**
      * Detaches the server process from the current process, enabling the current process to exit.
      *

distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerCliTests.java

@@ -93,15 +93,23 @@ public void testPositionalArgs() throws Exception {
         assertUsage(containsString(prefix + "[foo]"), "-E", "foo=bar", "foo", "-E", "baz=qux");
     }
 
-    public void testPidFile() throws Exception {
+    public void assertPidFile(String option) throws Exception {
         Path tmpDir = createTempDir();
         Path pidFileArg = tmpDir.resolve("pid");
-        assertUsage(containsString("Option p/pidfile requires an argument"), "-p");
-        argsValidator = args -> assertThat(args.pidFile().toString(), equalTo(pidFileArg.toString()));
-        terminal.reset();
-        assertOk("-p", pidFileArg.toString());
         terminal.reset();
-        assertOk("--pidfile", pidFileArg.toString());
+        argsValidator = args -> assertThat(args.pidFile().toString(), equalTo(pidFileArg.toString()));
+        assertOk(option, pidFileArg.toString());
+    }
+
+    public void testPidFile() throws Exception {
+        assertPidFile("-p");
+        assertPidFile("--pidfile");
+
+        assertUsage(containsString("Option p/pidfile requires an argument"), "-p");
+        Path pidParentFile = createTempFile();
+        assertUsage(containsString("exists but is not a directory"), "-p", pidParentFile.resolve("pid").toString());
+        assertUsage(containsString("exists but is not a regular file"), "-p", createTempDir().toString());
+
     }
 
     public void assertDaemonized(boolean daemonized, String... args) throws Exception {
@@ -300,6 +308,11 @@ private class MockServerProcess extends ServerProcess {
             super(null, null);
         }
 
+        @Override
+        public long pid() {
+            return 12345;
+        }
+
         @Override
         public void detach() {
             assert detachCalled == false;

distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java

@@ -79,7 +79,7 @@ interface ProcessValidator {
     }
 
     void runForeground() throws Exception {
-        var server = startProcess(false, false, null, "");
+        var server = startProcess(false, false, "");
         server.waitFor();
     }
 
@@ -190,10 +190,10 @@ public Process destroyForcibly() {
         }
     }
 
-    ServerProcess startProcess(boolean daemonize, boolean quiet, Path pidFile, String keystorePassword) throws Exception {
+    ServerProcess startProcess(boolean daemonize, boolean quiet, String keystorePassword) throws Exception {
         var pinfo = new ProcessInfo(Map.copyOf(sysprops), Map.copyOf(envVars), esHomeDir);
         SecureString password = new SecureString(keystorePassword.toCharArray());
-        var args = new ServerArgs(daemonize, quiet, pidFile, password, nodeSettings.build(), esHomeDir.resolve("config"));
+        var args = new ServerArgs(daemonize, quiet, null, password, nodeSettings.build(), esHomeDir.resolve("config"));
         ServerProcess.ProcessStarter starter = pb -> {
             if (processValidator != null) {
                 processValidator.validate(pb);
@@ -220,6 +220,12 @@ public void testProcessBuilder() throws Exception {
         assertThat(terminal.getErrorOutput(), containsString("stderr message"));
     }
 
+    public void testPid() throws Exception {
+        var server = startProcess(true, false, "");
+        assertThat(server.pid(), equalTo(12345L));
+        server.stop();
+    }
+
     public void testBootstrapError() throws Exception {
         mainCallback = (args, stdin, stderr, exitCode) -> {
             stderr.println(BootstrapInfo.USER_EXCEPTION_MARKER + "a bootstrap exception");
@@ -367,7 +373,7 @@ public void testDetach() throws Exception {
             // will block until stdin closed manually after test
             assertThat(stdin.read(), equalTo(-1));
         };
-        var server = startProcess(true, false, null, "");
+        var server = startProcess(true, false, "");
         server.detach();
         assertThat(terminal.getErrorOutput(), containsString("final message"));
         server.stop(); // this should be a noop, and will fail the stdin read assert above if shutdown sent
@@ -381,7 +387,7 @@ public void testStop() throws Exception {
             nonInterruptibleVoid(mainReady::await);
             stderr.println("final message");
         };
-        var server = startProcess(false, false, null, "");
+        var server = startProcess(false, false, "");
         mainReady.countDown();
         server.stop();
         assertThat(process.main.isDone(), is(true)); // stop should have waited
@@ -396,7 +402,7 @@ public void testWaitFor() throws Exception {
             assertThat(stdin.read(), equalTo((int) BootstrapInfo.SERVER_SHUTDOWN_MARKER));
             stderr.println("final message");
         };
-        var server = startProcess(false, false, null, "");
+        var server = startProcess(false, false, "");
         new Thread(() -> {
             // simulate stop run as shutdown hook in another thread, eg from Ctrl-C
             nonInterruptibleVoid(mainReady::await);
@@ -417,7 +423,7 @@ public void testProcessDies() throws Exception {
             nonInterruptibleVoid(mainExit::await);
             exitCode.set(-9);
         };
-        var server = startProcess(false, false, null, "");
+        var server = startProcess(false, false, "");
         nonInterruptibleVoid(mainReady::await);
         process.processStderr.close(); // mimic pipe break if cli process dies
         mainExit.countDown();

qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java

@@ -77,7 +77,6 @@ public void testEnvironmentPaths() throws Exception {
         );
         settingsBuilder.put(Environment.PATH_SHARED_DATA_SETTING.getKey(), esHome.resolve("custom").toString());
         settingsBuilder.put(Environment.PATH_LOGS_SETTING.getKey(), esHome.resolve("logs").toString());
-        settingsBuilder.put(Environment.NODE_PIDFILE_SETTING.getKey(), esHome.resolve("test.pid").toString());
         Settings settings = settingsBuilder.build();
 
         Path fakeTmpDir = createTempDir();
@@ -123,8 +122,6 @@ public void testEnvironmentPaths() throws Exception {
         assertExactPermissions(new FilePermission(environment.logsFile().toString(), "read,readlink,write,delete"), permissions);
         // temp dir: r/w
         assertExactPermissions(new FilePermission(fakeTmpDir.toString(), "read,readlink,write,delete"), permissions);
-        // PID file: delete only (for the shutdown hook)
-        assertExactPermissions(new FilePermission(environment.pidFile().toString(), "delete"), permissions);
     }
 
     public void testDuplicateDataPaths() throws IOException {

server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

@@ -19,7 +19,6 @@
 import org.elasticsearch.Version;
 import org.elasticsearch.bootstrap.plugins.PluginsManager;
 import org.elasticsearch.cli.UserException;
-import org.elasticsearch.common.PidFile;
 import org.elasticsearch.common.filesystem.FileSystemNatives;
 import org.elasticsearch.common.inject.CreationException;
 import org.elasticsearch.common.logging.LogConfigurator;
@@ -243,15 +242,11 @@ protected void validateNodeBeforeAcceptingRequests(
     // visible for tests
 
     private static Environment createEnvironment(
-        final Path pidFile,
         final SecureSettings secureSettings,
         final Settings initialSettings,
         final Path configPath
     ) {
         Settings.Builder builder = Settings.builder();
-        if (pidFile != null) {
-            builder.put(Environment.NODE_PIDFILE_SETTING.getKey(), pidFile);
-        }
         builder.put(initialSettings);
         if (secureSettings != null) {
             builder.setSecureSettings(secureSettings);
@@ -287,21 +282,16 @@ static void stop() throws IOException {
     /**
      * This method is invoked by {@link Elasticsearch#main(String[])} to startup elasticsearch.
      */
-    static void init(
-        final boolean foreground,
-        final Path pidFile,
-        final boolean quiet,
-        final Environment initialEnv,
-        SecureString keystorePassword
-    ) throws BootstrapException, NodeValidationException, UserException {
+    static void init(final boolean foreground, final boolean quiet, final Environment initialEnv, SecureString keystorePassword)
+        throws BootstrapException, NodeValidationException, UserException {
         // force the class initializer for BootstrapInfo to run before
         // the security manager is installed
         BootstrapInfo.init();
 
         INSTANCE = new Bootstrap();
 
         final SecureSettings keystore = BootstrapUtil.loadSecureSettings(initialEnv, keystorePassword);
-        final Environment environment = createEnvironment(pidFile, keystore, initialEnv.settings(), initialEnv.configFile());
+        final Environment environment = createEnvironment(keystore, initialEnv.settings(), initialEnv.configFile());
 
         BootstrapInfo.setConsole(getConsole(environment));
 
@@ -311,13 +301,6 @@ static void init(
         } catch (IOException e) {
             throw new BootstrapException(e);
         }
-        if (environment.pidFile() != null) {
-            try {
-                PidFile.create(environment.pidFile(), true);
-            } catch (IOException e) {
-                throw new BootstrapException(e);
-            }
-        }
 
         try {
             // fail if somebody replaced the lucene jars

server/src/main/java/org/elasticsearch/bootstrap/BootstrapException.java

@@ -8,14 +8,12 @@
 
 package org.elasticsearch.bootstrap;
 
-import java.nio.file.Path;
-
 /**
  * Wrapper exception for checked exceptions thrown during the bootstrap process. Methods invoked
  * during bootstrap should explicitly declare the checked exceptions that they can throw, rather
  * than declaring the top-level checked exception {@link Exception}. This exception exists to wrap
  * these checked exceptions so that
- * {@link Bootstrap#init(boolean, Path, boolean, org.elasticsearch.env.Environment, org.elasticsearch.common.settings.SecureString)}
+ * {@link Bootstrap#init(boolean, boolean, org.elasticsearch.env.Environment, org.elasticsearch.common.settings.SecureString)}
  * does not have to declare all of these checked exceptions.
  */
 class BootstrapException extends Exception {

server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java

@@ -22,6 +22,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.PrintStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.security.Permission;
 import java.security.Security;
@@ -61,9 +62,9 @@ public void checkPermission(Permission perm) {
         try {
             final var in = new InputStreamStreamInput(System.in);
             final ServerArgs serverArgs = new ServerArgs(in);
+            initPidFile(serverArgs.pidFile());
             elasticsearch.init(
                 serverArgs.daemonize(),
-                serverArgs.pidFile(),
                 serverArgs.quiet(),
                 new Environment(serverArgs.nodeSettings(), serverArgs.configDir()),
                 serverArgs.keystorePassword()
@@ -171,6 +172,31 @@ private static void startCliMonitorThread(InputStream stdin) {
         }).start();
     }
 
+    /**
+     * Writes the current process id into the given pidfile, if not null. The pidfile is cleaned up on system exit.
+     *
+     * @param pidFile A path to a file, or null of no pidfile should be written
+     */
+    private static void initPidFile(Path pidFile) throws IOException {
+        if (pidFile == null) {
+            return;
+        }
+        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+            if (Files.exists(pidFile)) {
+                try {
+                    Files.delete(pidFile);
+                } catch (IOException e) {
+                    // ignore, nothing we can do because are shutting down
+                }
+            }
+        }, "elasticsearch[pidfile-cleanup]"));
+
+        if (Files.exists(pidFile.getParent()) == false) {
+            Files.createDirectories(pidFile.getParent());
+        }
+        Files.writeString(pidFile, Long.toString(ProcessHandle.current().pid()));
+    }
+
     private static void overrideDnsCachePolicyProperties() {
         for (final String property : new String[] { "networkaddress.cache.ttl", "networkaddress.cache.negative.ttl" }) {
             final String overrideProperty = "es." + property;
@@ -186,10 +212,10 @@ private static void overrideDnsCachePolicyProperties() {
         }
     }
 
-    void init(final boolean daemonize, final Path pidFile, final boolean quiet, Environment initialEnv, SecureString keystorePassword)
+    void init(final boolean daemonize, final boolean quiet, Environment initialEnv, SecureString keystorePassword)
         throws NodeValidationException, UserException {
         try {
-            Bootstrap.init(daemonize == false, pidFile, quiet, initialEnv, keystorePassword);
+            Bootstrap.init(daemonize == false, quiet, initialEnv, keystorePassword);
         } catch (BootstrapException | RuntimeException e) {
             // format exceptions to the console in a special way
             // to avoid 2MB stacktraces from guice, etc.

server/src/main/java/org/elasticsearch/bootstrap/Security.java

@@ -245,10 +245,6 @@ static void addFilePermissions(Permissions policy, Environment environment) thro
         for (Path path : environment.repoFiles()) {
             addDirectoryPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete", false);
         }
-        if (environment.pidFile() != null) {
-            // we just need permission to remove the file if its elsewhere.
-            addSingleFilePath(policy, environment.pidFile(), "delete");
-        }
     }
 
     /**