From 2d0e9046fb894e220083cd81fa77b78e1ff71144 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Thu, 18 Apr 2019 11:54:58 +0200 Subject: [PATCH 1/2] Fix SnapshotResiliencyTest Repo Consistency Check * Due to the random creation of an empty `extra0` file by the Lucene mockFS we see broken tests because we use the existence of an index folder in assertions and the index deletion doesn't go through if there's extra files in an index folder * Fixed by removing the `extra0` file and resulting empty directory trees before asserting repo consistency * Closes #41326 --- .../snapshots/SnapshotResiliencyTests.java | 59 +++++++++++++++++-- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java index b2e7562286caa..5ec32f63b0630 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java @@ -167,10 +167,16 @@ import org.junit.After; import org.junit.Before; +import java.io.FileNotFoundException; import java.io.IOException; import java.nio.ByteBuffer; +import java.nio.file.DirectoryNotEmptyException; +import java.nio.file.FileVisitResult; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; import java.util.Collection; import java.util.Collections; import java.util.Comparator; @@ -378,7 +384,6 @@ public void testSnapshotWithNodeDisconnects() { assertThat(snapshotIds, hasSize(1)); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/41326") public void testConcurrentSnapshotCreateAndDelete() { setupTestCluster(randomFrom(1, 3, 5), randomIntBetween(2, 10)); @@ -525,10 +530,11 @@ public void run() { private void assertNoStaleRepositoryData() throws IOException { final Path repoPath = tempDir.resolve("repo").toAbsolutePath(); final List repos; - try (Stream reposDir = Files.list(repoPath)) { + try (Stream reposDir = repoFilesByPrefix(repoPath)) { repos = reposDir.filter(s -> s.getFileName().toString().startsWith("extra") == false).collect(Collectors.toList()); } for (Path repoRoot : repos) { + cleanupEmptyTrees(repoRoot); final Path latestIndexGenBlob = repoRoot.resolve("index.latest"); assertTrue("Could not find index.latest blob for repo at [" + repoRoot + ']', Files.exists(latestIndexGenBlob)); final long latestGen = ByteBuffer.wrap(Files.readAllBytes(latestIndexGenBlob)).getLong(0); @@ -544,8 +550,37 @@ private void assertNoStaleRepositoryData() throws IOException { } } + // Lucene's mock file system randomly generates empty `extra0` files that break the deletion of blob-store directories. + // We clean those up here before checking a blob-store for stale files in this test. + private void cleanupEmptyTrees(Path repoPath) { + try { + Files.walkFileTree(repoPath, new SimpleFileVisitor<>() { + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + if (file.getFileName().toString().startsWith("extra")) { + Files.delete(file); + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + try { + Files.delete(dir); + } catch (DirectoryNotEmptyException e) { + // We're only interested in deleting empty trees here, just ignore directories with content + } + return FileVisitResult.CONTINUE; + } + }); + } catch (IOException e) { + throw new AssertionError(e); + } + } + private static void assertIndexGenerations(Path repoRoot, long latestGen) throws IOException { - try (Stream repoRootBlobs = Files.list(repoRoot)) { + try (Stream repoRootBlobs = repoFilesByPrefix(repoRoot)) { final long[] indexGenerations = repoRootBlobs.filter(p -> p.getFileName().toString().startsWith("index-")) .map(p -> p.getFileName().toString().replace("index-", "")) .mapToLong(Long::parseLong).sorted().toArray(); @@ -557,7 +592,7 @@ private static void assertIndexGenerations(Path repoRoot, long latestGen) throws private static void assertIndexUUIDs(Path repoRoot, RepositoryData repositoryData) throws IOException { final List expectedIndexUUIDs = repositoryData.getIndices().values().stream().map(IndexId::getId).collect(Collectors.toList()); - try (Stream indexRoots = Files.list(repoRoot.resolve("indices"))) { + try (Stream indexRoots = repoFilesByPrefix(repoRoot.resolve("indices"))) { final List foundIndexUUIDs = indexRoots.filter(s -> s.getFileName().toString().startsWith("extra") == false) .map(p -> p.getFileName().toString()).collect(Collectors.toList()); assertThat(foundIndexUUIDs, containsInAnyOrder(expectedIndexUUIDs.toArray(Strings.EMPTY_ARRAY))); @@ -568,7 +603,7 @@ private static void assertSnapshotUUIDs(Path repoRoot, RepositoryData repository final List expectedSnapshotUUIDs = repositoryData.getSnapshotIds().stream().map(SnapshotId::getUUID).collect(Collectors.toList()); for (String prefix : new String[]{"snap-", "meta-"}) { - try (Stream repoRootBlobs = Files.list(repoRoot)) { + try (Stream repoRootBlobs = repoFilesByPrefix(repoRoot)) { final Collection foundSnapshotUUIDs = repoRootBlobs.filter(p -> p.getFileName().toString().startsWith(prefix)) .map(p -> p.getFileName().toString().replace(prefix, "").replace(".dat", "")) .collect(Collectors.toSet()); @@ -577,6 +612,20 @@ private static void assertSnapshotUUIDs(Path repoRoot, RepositoryData repository } } + /** + * List contents of a blob path and return an empty stream if the path doesn't exist. + * @param prefix Path to find children for + * @return stream of child paths + * @throws IOException on failure + */ + private static Stream repoFilesByPrefix(Path prefix) throws IOException { + try { + return Files.list(prefix); + } catch (FileNotFoundException | NoSuchFileException e) { + return Stream.empty(); + } + } + private void clearDisruptionsAndAwaitSync() { testClusterNodes.clearNetworkDisruptions(); runUntil(() -> { From 958ce345de79f8a1a56093453b00804cb1727cdb Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Thu, 18 Apr 2019 14:36:13 +0200 Subject: [PATCH 2/2] CR: fix comment --- .../org/elasticsearch/snapshots/SnapshotResiliencyTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java index 5ec32f63b0630..2661f958fc2a2 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java @@ -551,7 +551,7 @@ private void assertNoStaleRepositoryData() throws IOException { } // Lucene's mock file system randomly generates empty `extra0` files that break the deletion of blob-store directories. - // We clean those up here before checking a blob-store for stale files in this test. + // We clean those up here before checking a blob-store for stale files. private void cleanupEmptyTrees(Path repoPath) { try { Files.walkFileTree(repoPath, new SimpleFileVisitor<>() {