From 9f1d11696726bbe54a7a98a6c7fa0c7febf60c63 Mon Sep 17 00:00:00 2001 From: Boaz Leskes Date: Tue, 1 Aug 2017 15:41:27 +0200 Subject: [PATCH] Node should start up despite of a lingering `.es_temp_file` (#21210) When ES starts up we verify we can write to all data folders and that they support atomic moves. We do so by creating and deleting temp files. If for some reason the files was successfully created but not successfully deleted, we still shut down correctly but subsequent start attempts will fail with a file already exists exception. This commit makes sure to first clean any existing temporary files. Superseeds #21007 --- .../elasticsearch/env/NodeEnvironment.java | 23 +++++++------ .../env/NodeEnvironmentTests.java | 34 +++++++++++++++++++ .../env/NodeEnvironmentEvilTests.java | 5 ++- 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java b/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java index 91cb96d60d7..ecebe411534 100644 --- a/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java +++ b/core/src/main/java/org/elasticsearch/env/NodeEnvironment.java @@ -901,11 +901,12 @@ public final class NodeEnvironment implements Closeable { final NodePath[] nodePaths = nodePaths(); for (NodePath nodePath : nodePaths) { assert Files.isDirectory(nodePath.path) : nodePath.path + " is not a directory"; - final Path src = nodePath.path.resolve("__es__.tmp"); - final Path target = nodePath.path.resolve("__es__.final"); + final Path src = nodePath.path.resolve(TEMP_FILE_NAME + ".tmp"); + final Path target = nodePath.path.resolve(TEMP_FILE_NAME + ".final"); try { + Files.deleteIfExists(src); Files.createFile(src); - Files.move(src, target, StandardCopyOption.ATOMIC_MOVE); + Files.move(src, target, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); } catch (AtomicMoveNotSupportedException ex) { throw new IllegalStateException("atomic_move is not supported by the filesystem on path [" + nodePath.path @@ -1005,19 +1006,19 @@ public final class NodeEnvironment implements Closeable { } } + // package private for testing + static final String TEMP_FILE_NAME = ".es_temp_file"; + private static void tryWriteTempFile(Path path) throws IOException { if (Files.exists(path)) { - Path resolve = path.resolve(".es_temp_file"); - boolean tempFileCreated = false; + Path resolve = path.resolve(TEMP_FILE_NAME); try { + // delete any lingering file from a previous failure + Files.deleteIfExists(resolve); Files.createFile(resolve); - tempFileCreated = true; + Files.delete(resolve); } catch (IOException ex) { - throw new IOException("failed to write in data directory [" + path + "] write permission is required", ex); - } finally { - if (tempFileCreated) { - Files.deleteIfExists(resolve); - } + throw new IOException("failed to test writes in data directory [" + path + "] write permission is required", ex); } } } diff --git a/core/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java b/core/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java index f067212caaf..42cb4a5811b 100644 --- a/core/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java +++ b/core/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java @@ -34,6 +34,7 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.IndexSettingsModule; import java.io.IOException; +import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; @@ -416,6 +417,39 @@ public class NodeEnvironmentTests extends ESTestCase { env.close(); } + public void testExistingTempFiles() throws IOException { + String[] paths = tmpPaths(); + // simulate some previous left over temp files + for (String path : randomSubsetOf(randomIntBetween(1, paths.length), paths)) { + final Path nodePath = NodeEnvironment.resolveNodePath(PathUtils.get(path), 0); + Files.createDirectories(nodePath); + Files.createFile(nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME)); + if (randomBoolean()) { + Files.createFile(nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".tmp")); + } + if (randomBoolean()) { + Files.createFile(nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".final")); + } + } + NodeEnvironment env = newNodeEnvironment(paths, Settings.EMPTY); + try { + env.ensureAtomicMoveSupported(); + } catch (AtomicMoveNotSupportedException e) { + // that's OK :) + } + env.close(); + // check we clean up + for (String path: paths) { + final Path nodePath = NodeEnvironment.resolveNodePath(PathUtils.get(path), 0); + final Path tempFile = nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME); + assertFalse(tempFile + " should have been cleaned", Files.exists(tempFile)); + final Path srcTempFile = nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".src"); + assertFalse(srcTempFile + " should have been cleaned", Files.exists(srcTempFile)); + final Path targetTempFile = nodePath.resolve(NodeEnvironment.TEMP_FILE_NAME + ".target"); + assertFalse(targetTempFile + " should have been cleaned", Files.exists(targetTempFile)); + } + } + /** Converts an array of Strings to an array of Paths, adding an additional child if specified */ private Path[] stringsToPaths(String[] strings, String additional) { Path[] locations = new Path[strings.length]; diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/env/NodeEnvironmentEvilTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/env/NodeEnvironmentEvilTests.java index ccf5e1b105e..3eebf4a2f64 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/env/NodeEnvironmentEvilTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/env/NodeEnvironmentEvilTests.java @@ -30,7 +30,6 @@ import java.nio.file.Path; import java.nio.file.attribute.PosixFileAttributeView; import java.nio.file.attribute.PosixFilePermission; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; public class NodeEnvironmentEvilTests extends ESTestCase { @@ -75,7 +74,7 @@ public class NodeEnvironmentEvilTests extends ESTestCase { IOException ioException = expectThrows(IOException.class, () -> { new NodeEnvironment(build, new Environment(build)); }); - assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to write in data directory")); + assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to test writes in data directory")); } } @@ -100,7 +99,7 @@ public class NodeEnvironmentEvilTests extends ESTestCase { IOException ioException = expectThrows(IOException.class, () -> { new NodeEnvironment(build, new Environment(build)); }); - assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to write in data directory")); + assertTrue(ioException.getMessage(), ioException.getMessage().startsWith("failed to test writes in data directory")); } } }