Use a marker file when removing a plugin

Today when removing a plugin, we attempt to move the plugin directory to
a temporary directory and then delete that directory from the
filesystem. We do this to avoid a plugin being in a half-removed
state. We previously tried an atomic move, and fell back to a non-atomic
move if that failed. Atomic moves can fail on union filesystems when the
plugin directory is not in the top layer of the
filesystem. Interestingly, the regular move can fail as well. This is
because when the JDK is executing such a move, it first tries to rename
the source directory to the target directory and if this fails with
EXDEV (as in the case of an atomic move failing), it falls back to
copying the source to the target, and then attempts to rmdir the
source. The bug here is that the JDK never deleted the contents of the
source so the rmdir will always fail (except in the case of an empty
directory).

Given all this silliness, we were inspired to find a different
strategy. The strategy is simple. We will add a marker file to the
plugin directory that indicates the plugin is in a state of
removal. This file will be the last file out the door during removal. If
this file exists during startup, we fail startup.

Relates #24252

core/src/main/java/org/elasticsearch/plugins/PluginsService.java

@@ -20,8 +20,6 @@
 package org.elasticsearch.plugins;
 
 import org.apache.logging.log4j.Logger;
-import org.apache.logging.log4j.message.ParameterizedMessage;
-import org.apache.logging.log4j.util.Supplier;
 import org.apache.lucene.analysis.util.CharFilterFactory;
 import org.apache.lucene.analysis.util.TokenFilterFactory;
 import org.apache.lucene.analysis.util.TokenizerFactory;
@@ -36,7 +34,6 @@ import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.component.LifecycleComponent;
 import org.elasticsearch.common.inject.Module;
-import org.elasticsearch.common.io.FileSystemUtils;
 import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
@@ -45,21 +42,19 @@ import org.elasticsearch.index.IndexModule;
 import org.elasticsearch.threadpool.ExecutorBuilder;
 
 import java.io.IOException;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -310,6 +305,19 @@ public class PluginsService extends AbstractComponent {
                     throw new IllegalStateException("Could not load plugin descriptor for existing plugin ["
                         + plugin.getFileName() + "]. Was the plugin built before 2.0?", e);
                 }
+                /*
+                 * Check for the existence of a marker file that indicates the plugin is in a garbage state from a failed attempt to remove
+                 * the plugin.
+                 */
+                final Path removing = plugin.resolve(".removing-" + info.getName());
+                if (Files.exists(removing)) {
+                    final String message = String.format(
+                            Locale.ROOT,
+                            "found file [%s] from a failed attempt to remove the plugin [%s]; execute [elasticsearch-plugin remove %2$s]",
+                            removing,
+                            info.getName());
+                    throw new IllegalStateException(message);
+                }
 
                 Set<URL> urls = new LinkedHashSet<>();
                 try (DirectoryStream<Path> jarStream = Files.newDirectoryStream(plugin, "*.jar")) {

core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.plugins;
 
 import org.apache.lucene.util.LuceneTestCase;
+import org.elasticsearch.Version;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.index.IndexModule;
@@ -30,6 +31,7 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Locale;
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.hasToString;
@@ -121,4 +123,32 @@ public class PluginsServiceTests extends ESTestCase {
         assertThat(e, hasToString(containsString(expected)));
     }
 
+    public void testStartupWithRemovingMarker() throws IOException {
+        final Path home = createTempDir();
+        final Settings settings =
+                Settings.builder()
+                        .put(Environment.PATH_HOME_SETTING.getKey(), home)
+                        .build();
+        final Path fake = home.resolve("plugins").resolve("fake");
+        Files.createDirectories(fake);
+        Files.createFile(fake.resolve("plugin.jar"));
+        final Path removing = fake.resolve(".removing-fake");
+        Files.createFile(fake.resolve(".removing-fake"));
+        PluginTestUtil.writeProperties(
+                fake,
+                "description", "fake",
+                "name", "fake",
+                "version", "1.0.0",
+                "elasticsearch.version", Version.CURRENT.toString(),
+                "java.version", System.getProperty("java.specification.version"),
+                "classname", "Fake",
+                "has.native.controller", "false");
+        final IllegalStateException e = expectThrows(IllegalStateException.class, () -> newPluginsService(settings));
+        final String expected = String.format(
+                Locale.ROOT,
+                "found file [%s] from a failed attempt to remove the plugin [fake]; execute [elasticsearch-plugin remove fake]",
+                removing);
+        assertThat(e, hasToString(containsString(expected)));
+    }
+
 }

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java

@@ -19,15 +19,6 @@
 
 package org.elasticsearch.plugins;
 
-import java.io.IOException;
-import java.nio.file.AtomicMoveNotSupportedException;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.StandardCopyOption;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Locale;
-
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
 import org.apache.lucene.util.IOUtils;
@@ -38,6 +29,14 @@ import org.elasticsearch.cli.UserException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.env.Environment;
 
+import java.io.IOException;
+import java.nio.file.FileAlreadyExistsException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+
 import static org.elasticsearch.cli.Terminal.Verbosity.VERBOSE;
 
 /**
@@ -101,20 +100,31 @@ class RemovePluginCommand extends EnvironmentAwareCommand {
         }
 
         terminal.println(VERBOSE, "removing [" + pluginDir + "]");
-        final Path tmpPluginDir = env.pluginsFile().resolve(".removing-" + pluginName);
+         /*
+         * We are going to create a marker file in the plugin directory that indicates that this plugin is a state of removal. If the
+         * removal fails, the existence of this marker file indicates that the plugin is in a garbage state. We check for existence of this
+         * marker file during startup so that we do not startup with plugins in such a garbage state.
+         */
+        final Path removing = pluginDir.resolve(".removing-" + pluginName);
+        /*
+         * Add the contents of the plugin directory before creating the marker file and adding it to the list of paths to be deleted so
+         * that the marker file is the last file to be deleted.
+         */
+        Files.list(pluginDir).forEach(pluginPaths::add);
         try {
-            Files.move(pluginDir, tmpPluginDir, StandardCopyOption.ATOMIC_MOVE);
-        } catch (final AtomicMoveNotSupportedException e) {
+            Files.createFile(removing);
+        } catch (final FileAlreadyExistsException e) {
             /*
-             * On a union file system if the plugin that we are removing is not installed on the
-             * top layer then atomic move will not be supported. In this case, we fall back to a
-             * non-atomic move.
+             * We need to suppress the marker file already existing as we could be in this state if a previous removal attempt failed and
+             * the user is attempting to remove the plugin again.
              */
-            Files.move(pluginDir, tmpPluginDir);
+            terminal.println(VERBOSE, "marker file [" + removing + "] already exists");
         }
-        pluginPaths.add(tmpPluginDir);
-
+        // now add the marker file
+        pluginPaths.add(removing);
         IOUtils.rm(pluginPaths.toArray(new Path[pluginPaths.size()]));
+        // at this point, the plugin directory is empty and we can execute a simple directory removal
+        Files.delete(pluginDir);
 
         /*
          * We preserve the config files in case the user is upgrading the plugin, but we print a
@@ -124,8 +134,7 @@ class RemovePluginCommand extends EnvironmentAwareCommand {
         if (Files.exists(pluginConfigDir)) {
             final String message = String.format(
                     Locale.ROOT,
-                    "-> preserving plugin config files [%s] in case of upgrade; "
-                            + "delete manually if not needed",
+                    "-> preserving plugin config files [%s] in case of upgrade; delete manually if not needed",
                     pluginConfigDir);
             terminal.println(message);
         }

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java

@@ -34,8 +34,6 @@ import java.io.StringReader;
 import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.HashMap;
-import java.util.Map;
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.not;
@@ -159,6 +157,13 @@ public class RemovePluginCommandTests extends ESTestCase {
         assertEquals("plugin name is required", e.getMessage());
     }
 
+    public void testRemoveWhenRemovingMarker() throws Exception {
+        Files.createDirectory(env.pluginsFile().resolve("fake"));
+        Files.createFile(env.pluginsFile().resolve("fake").resolve("plugin.jar"));
+        Files.createFile(env.pluginsFile().resolve("fake").resolve(".removing-fake"));
+        removePlugin("fake", home);
+    }
+
     private String expectedConfigDirPreservedMessage(final Path configDir) {
         return "-> preserving plugin config files [" + configDir + "] in case of upgrade; delete manually if not needed";
     }