Skip to content

Commit

Permalink
Fix testSnapshotWithStuckNode (elastic#102398)
Browse files Browse the repository at this point in the history
This test sometimes relies on repository cleanup to remove all but the
`index.latest` and `index-N` blobs, but in fact repo cleanup will leave
behind the `index-(N-1)` blob too. This commit relaxes the test to
account for this, but then strengthens it to assert that the blobs left
in the repo are exactly the ones we expect.

Closes elastic#101573
  • Loading branch information
DaveCTurner committed Nov 21, 2023
1 parent d9865bc commit 8b13e1d
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFutureThrows;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.empty;
Expand All @@ -104,6 +105,7 @@
import static org.hamcrest.Matchers.lessThan;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.startsWith;

@ClusterScope(scope = Scope.TEST, numDataNodes = 0)
public class DedicatedClusterSnapshotRestoreIT extends AbstractSnapshotIntegTestCase {
Expand Down Expand Up @@ -199,13 +201,28 @@ public void testSnapshotWithStuckNode() throws Exception {
() -> clusterAdmin().prepareGetSnapshots("test-repo").setSnapshots("test-snap").execute().actionGet()
);

logger.info("--> Go through a loop of creating and deleting a snapshot to trigger repository cleanup");
logger.info("--> trigger repository cleanup");
clusterAdmin().prepareCleanupRepository("test-repo").get();

// Expect two files to remain in the repository:
// (1) index-(N+1)
// (2) index-latest
assertFileCount(repo, 2);
// Expect two or three files to remain in the repository:
// (1) index-latest
// (2) index-(N+1)
// (3) index-N (maybe: a fully successful deletion removes this, but cleanup does not, see #100718)

final var blobPaths = getAllFilesInDirectoryAndDescendants(repo);
final var blobPathsString = blobPaths.toString();
assertTrue(blobPathsString, blobPaths.remove(repo.resolve(BlobStoreRepository.INDEX_LATEST_BLOB)));
assertThat(blobPathsString, blobPaths, anyOf(hasSize(1), hasSize(2)));
final var repoGenerations = blobPaths.stream().mapToLong(blobPath -> {
final var blobName = repo.relativize(blobPath).toString();
assertThat(blobPathsString, blobName, startsWith(BlobStoreRepository.INDEX_FILE_PREFIX));
return Long.parseLong(blobName.substring(BlobStoreRepository.INDEX_FILE_PREFIX.length()));
}).toArray();

if (repoGenerations.length == 2) {
assertEquals(blobPathsString, 1, Math.abs(repoGenerations[0] - repoGenerations[1]));
}

logger.info("--> done");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,14 @@ public static long getFailureCount(String repository) {
}

public static void assertFileCount(Path dir, int expectedCount) throws IOException {
final List<Path> found = getAllFilesInDirectoryAndDescendants(dir);
assertEquals("Unexpected file count, found: [" + found + "].", expectedCount, found.size());
}

protected static List<Path> getAllFilesInDirectoryAndDescendants(Path dir) throws IOException {
final List<Path> found = new ArrayList<>();
forEachFileRecursively(dir, ((path, basicFileAttributes) -> found.add(path)));
assertEquals("Unexpected file count, found: [" + found + "].", expectedCount, found.size());
return found;
}

protected void stopNode(final String node) throws IOException {
Expand Down

0 comments on commit 8b13e1d

Please sign in to comment.