Skip to content

Commit dbfbcc4

Browse files
original-brownbearoriginal-brownbear
committed
Recursively Delete Unreferenced Index Directories (elastic#42189)
* Use ability to list child "folders" in the blob store to implement recursive delete on all stale index folders when cleaning up instead of using the diff between two `RepositoryData` instances to cover aborted deletes * Runs after ever delete operation * Relates elastic#13159 (fixing most of this issues caused by unreferenced indices, leaving some meta files to be cleaned up only)
1 parent 2176d09 commit dbfbcc4

File tree

12 files changed

+345
-149
lines changed

12 files changed

+345
-149
lines changed

‎plugins/repository-hdfs/src/test/java/org/elasticsearch/repositories/hdfs/HdfsTests.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,13 @@
2727
import org.elasticsearch.cluster.ClusterState;
2828
import org.elasticsearch.common.settings.Settings;
2929
import org.elasticsearch.plugins.Plugin;
30+
import org.elasticsearch.repositories.RepositoriesService;
3031
import org.elasticsearch.repositories.RepositoryException;
32+
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
33+
import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;
3134
import org.elasticsearch.snapshots.SnapshotState;
3235
import org.elasticsearch.test.ESSingleNodeTestCase;
36+
import org.elasticsearch.threadpool.ThreadPool;
3337

3438
import java.util.Collection;
3539

@@ -145,6 +149,9 @@ public void testSimpleWorkflow() {
145149
ClusterState clusterState = client.admin().cluster().prepareState().get().getState();
146150
assertThat(clusterState.getMetaData().hasIndex("test-idx-1"), equalTo(true));
147151
assertThat(clusterState.getMetaData().hasIndex("test-idx-2"), equalTo(false));
152+
final BlobStoreRepository repo =
153+
(BlobStoreRepository) getInstanceFromNode(RepositoriesService.class).repository("test-repo");
154+
BlobStoreTestUtil.assertConsistency(repo, repo.threadPool().executor(ThreadPool.Names.GENERIC));
148155
}
149156

150157
public void testMissingUri() {

‎plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryThirdPartyTests.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,12 @@
2626
import org.elasticsearch.common.settings.Settings;
2727
import org.elasticsearch.plugins.Plugin;
2828
import org.elasticsearch.repositories.AbstractThirdPartyRepositoryTestCase;
29+
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
2930
import org.elasticsearch.test.StreamsUtils;
3031

3132
import java.io.IOException;
3233
import java.util.Collection;
34+
import java.util.concurrent.Executor;
3335
import java.util.Map;
3436
import java.util.concurrent.TimeUnit;
3537

@@ -76,6 +78,20 @@ protected void createRepository(String repoName) {
7678
}
7779

7880
@Override
81+
protected boolean assertCorruptionVisible(BlobStoreRepository repo, Executor genericExec) throws Exception {
82+
// S3 is only eventually consistent for the list operations used by this assertions so we retry for 10 minutes assuming that
83+
// listing operations will become consistent within these 10 minutes.
84+
assertBusy(() -> assertTrue(super.assertCorruptionVisible(repo, genericExec)), 10L, TimeUnit.MINUTES);
85+
return true;
86+
}
87+
88+
@Override
89+
protected void assertConsistentRepository(BlobStoreRepository repo, Executor executor) throws Exception {
90+
// S3 is only eventually consistent for the list operations used by this assertions so we retry for 10 minutes assuming that
91+
// listing operations will become consistent within these 10 minutes.
92+
assertBusy(() -> super.assertConsistentRepository(repo, executor), 10L, TimeUnit.MINUTES);
93+
}
94+
7995
protected void assertBlobsByPrefix(BlobPath path, String prefix, Map<String, BlobMetaData> blobs) throws Exception {
8096
// AWS S3 is eventually consistent so we retry for 10 minutes assuming a list operation will never take longer than that
8197
// to become consistent.

‎server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java‎

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@
5757
import org.elasticsearch.common.settings.Settings;
5858
import org.elasticsearch.common.unit.ByteSizeUnit;
5959
import org.elasticsearch.common.unit.ByteSizeValue;
60-
import org.elasticsearch.common.util.set.Sets;
6160
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
6261
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
6362
import org.elasticsearch.common.xcontent.XContentFactory;
@@ -419,46 +418,68 @@ public void deleteSnapshot(SnapshotId snapshotId, long repositoryStateId, Action
419418
logger.warn(() -> new ParameterizedMessage("cannot read snapshot file [{}]", snapshotId), ex);
420419
}
421420
// Delete snapshot from the index file, since it is the maintainer of truth of active snapshots
422-
final RepositoryData repositoryData;
423421
final RepositoryData updatedRepositoryData;
422+
final Map<String, BlobContainer> foundIndices;
424423
try {
425-
repositoryData = getRepositoryData();
424+
final RepositoryData repositoryData = getRepositoryData();
426425
updatedRepositoryData = repositoryData.removeSnapshot(snapshotId);
426+
// Cache the indices that were found before writing out the new index-N blob so that a stuck master will never
427+
// delete an index that was created by another master node after writing this index-N blob.
428+
foundIndices = blobStore().blobContainer(basePath().add("indices")).children();
427429
writeIndexGen(updatedRepositoryData, repositoryStateId);
428430
} catch (Exception ex) {
429431
listener.onFailure(new RepositoryException(metadata.name(), "failed to delete snapshot [" + snapshotId + "]", ex));
430432
return;
431433
}
432434
final SnapshotInfo finalSnapshotInfo = snapshot;
433-
final Collection<IndexId> unreferencedIndices = Sets.newHashSet(repositoryData.getIndices().values());
434-
unreferencedIndices.removeAll(updatedRepositoryData.getIndices().values());
435435
try {
436436
blobContainer().deleteBlobsIgnoringIfNotExists(
437437
Arrays.asList(snapshotFormat.blobName(snapshotId.getUUID()), globalMetaDataFormat.blobName(snapshotId.getUUID())));
438438
} catch (IOException e) {
439439
logger.warn(() -> new ParameterizedMessage("[{}] Unable to delete global metadata files", snapshotId), e);
440440
}
441+
final Map<String, IndexId> survivingIndices = updatedRepositoryData.getIndices();
441442
deleteIndices(
442443
Optional.ofNullable(finalSnapshotInfo)
443-
.map(info -> info.indices().stream().map(repositoryData::resolveIndexId).collect(Collectors.toList()))
444+
.map(info -> info.indices().stream().filter(survivingIndices::containsKey)
445+
.map(updatedRepositoryData::resolveIndexId).collect(Collectors.toList()))
444446
.orElse(Collections.emptyList()),
445447
snapshotId,
446448
ActionListener.map(listener, v -> {
447-
try {
448-
blobStore().blobContainer(indicesPath()).deleteBlobsIgnoringIfNotExists(
449-
unreferencedIndices.stream().map(IndexId::getId).collect(Collectors.toList()));
450-
} catch (IOException e) {
451-
logger.warn(() ->
452-
new ParameterizedMessage(
453-
"[{}] indices {} are no longer part of any snapshots in the repository, " +
454-
"but failed to clean up their index folders.", metadata.name(), unreferencedIndices), e);
455-
}
449+
cleanupStaleIndices(foundIndices, survivingIndices);
456450
return null;
457451
})
458452
);
459453
}
460454
}
461455

456+
private void cleanupStaleIndices(Map<String, BlobContainer> foundIndices, Map<String, IndexId> survivingIndices) {
457+
try {
458+
final Set<String> survivingIndexIds = survivingIndices.values().stream()
459+
.map(IndexId::getId).collect(Collectors.toSet());
460+
for (Map.Entry<String, BlobContainer> indexEntry : foundIndices.entrySet()) {
461+
final String indexSnId = indexEntry.getKey();
462+
try {
463+
if (survivingIndexIds.contains(indexSnId) == false) {
464+
logger.debug("[{}] Found stale index [{}]. Cleaning it up", metadata.name(), indexSnId);
465+
indexEntry.getValue().delete();
466+
logger.debug("[{}] Cleaned up stale index [{}]", metadata.name(), indexSnId);
467+
}
468+
} catch (IOException e) {
469+
logger.warn(() -> new ParameterizedMessage(
470+
"[{}] index {} is no longer part of any snapshots in the repository, " +
471+
"but failed to clean up their index folders", metadata.name(), indexSnId), e);
472+
}
473+
}
474+
} catch (Exception e) {
475+
// TODO: We shouldn't be blanket catching and suppressing all exceptions here and instead handle them safely upstream.
476+
// Currently this catch exists as a stop gap solution to tackle unexpected runtime exceptions from implementations
477+
// bubbling up and breaking the snapshot functionality.
478+
assert false : e;
479+
logger.warn(new ParameterizedMessage("[{}] Exception during cleanup of stale indices", metadata.name()), e);
480+
}
481+
}
482+
462483
private void deleteIndices(List<IndexId> indices, SnapshotId snapshotId, ActionListener<Void> listener) {
463484
if (indices.isEmpty()) {
464485
listener.onResponse(null);
@@ -523,9 +544,9 @@ public SnapshotInfo finalizeSnapshot(final SnapshotId snapshotId,
523544
startTime, failure, System.currentTimeMillis(), totalShards, shardFailures,
524545
includeGlobalState, userMetadata);
525546
try {
547+
final RepositoryData updatedRepositoryData = getRepositoryData().addSnapshot(snapshotId, blobStoreSnapshot.state(), indices);
526548
snapshotFormat.write(blobStoreSnapshot, blobContainer(), snapshotId.getUUID());
527-
final RepositoryData repositoryData = getRepositoryData();
528-
writeIndexGen(repositoryData.addSnapshot(snapshotId, blobStoreSnapshot.state(), indices), repositoryStateId);
549+
writeIndexGen(updatedRepositoryData, repositoryStateId);
529550
} catch (FileAlreadyExistsException ex) {
530551
// if another master was elected and took over finalizing the snapshot, it is possible
531552
// that both nodes try to finalize the snapshot and write to the same blobs, so we just

‎server/src/test/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,13 @@
2020

2121
import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse;
2222
import org.elasticsearch.cluster.SnapshotsInProgress;
23+
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
2324
import org.elasticsearch.cluster.routing.allocation.decider.EnableAllocationDecider;
2425
import org.elasticsearch.common.settings.Settings;
2526
import org.elasticsearch.common.unit.TimeValue;
2627
import org.elasticsearch.plugins.Plugin;
2728
import org.elasticsearch.repositories.RepositoriesService;
29+
import org.elasticsearch.repositories.blobstore.BlobStoreTestUtil;
2830
import org.elasticsearch.snapshots.mockstore.MockRepository;
2931
import org.elasticsearch.test.ESIntegTestCase;
3032
import org.junit.After;
@@ -65,6 +67,32 @@ public void assertConsistentHistoryInLuceneIndex() throws Exception {
6567
internalCluster().assertConsistentHistoryBetweenTranslogAndLuceneIndex();
6668
}
6769

70+
private String skipRepoConsistencyCheckReason;
71+
72+
@After
73+
public void assertRepoConsistency() {
74+
if (skipRepoConsistencyCheckReason == null) {
75+
client().admin().cluster().prepareGetRepositories().get().repositories()
76+
.stream()
77+
.map(RepositoryMetaData::name)
78+
.forEach(name -> {
79+
final List<SnapshotInfo> snapshots = client().admin().cluster().prepareGetSnapshots(name).get().getSnapshots();
80+
// Delete one random snapshot to trigger repository cleanup.
81+
if (snapshots.isEmpty() == false) {
82+
client().admin().cluster().prepareDeleteSnapshot(name, randomFrom(snapshots).snapshotId().getName()).get();
83+
}
84+
BlobStoreTestUtil.assertRepoConsistency(internalCluster(), name);
85+
});
86+
} else {
87+
logger.info("--> skipped repo consistency checks because [{}]", skipRepoConsistencyCheckReason);
88+
}
89+
}
90+
91+
protected void disableRepoConsistencyCheck(String reason) {
92+
assertNotNull(reason);
93+
skipRepoConsistencyCheckReason = reason;
94+
}
95+
6896
public static long getFailureCount(String repository) {
6997
long failureCount = 0;
7098
for (RepositoriesService repositoriesService :

‎server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -723,6 +723,7 @@ public boolean clearData(String nodeName) {
723723
}
724724

725725
public void testRegistrationFailure() {
726+
disableRepoConsistencyCheck("This test does not create any data in the repository");
726727
logger.info("--> start first node");
727728
internalCluster().startNode();
728729
logger.info("--> start second node");
@@ -742,6 +743,7 @@ public void testRegistrationFailure() {
742743
}
743744

744745
public void testThatSensitiveRepositorySettingsAreNotExposed() throws Exception {
746+
disableRepoConsistencyCheck("This test does not create any data in the repository");
745747
Settings nodeSettings = Settings.EMPTY;
746748
logger.info("--> start two nodes");
747749
internalCluster().startNodes(2, nodeSettings);

‎server/src/test/java/org/elasticsearch/snapshots/MetadataLoadingDuringSnapshotRestoreIT.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,8 @@ public void testWhenMetadataAreLoaded() throws Exception {
144144
// Deleting a snapshot does not load the global metadata state but loads each index metadata
145145
assertAcked(client().admin().cluster().prepareDeleteSnapshot("repository", "snap").get());
146146
assertGlobalMetadataLoads("snap", 1);
147-
assertIndexMetadataLoads("snap", "docs", 5);
148-
assertIndexMetadataLoads("snap", "others", 4);
147+
assertIndexMetadataLoads("snap", "docs", 4);
148+
assertIndexMetadataLoads("snap", "others", 3);
149149
}
150150

151151
private void assertGlobalMetadataLoads(final String snapshot, final int times) {

‎server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,8 @@ public void testRepositoryAckTimeout() throws Exception {
184184
}
185185

186186
public void testRepositoryVerification() throws Exception {
187+
disableRepoConsistencyCheck("This test does not create any data in the repository.");
188+
187189
Client client = client();
188190

189191
Settings settings = Settings.builder()

‎server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ public void testSingleGetAfterRestore() throws Exception {
363363
assertThat(client.prepareGet(restoredIndexName, typeName, docId).get().isExists(), equalTo(true));
364364
}
365365

366-
public void testFreshIndexUUID() {
366+
public void testFreshIndexUUID() throws InterruptedException {
367367
Client client = client();
368368

369369
logger.info("--> creating repository");
@@ -541,7 +541,6 @@ public void testRestoreAliases() throws Exception {
541541
logger.info("--> check that aliases are not restored and existing aliases still exist");
542542
assertAliasesMissing(client.admin().indices().prepareAliasesExist("alias-123", "alias-1").get());
543543
assertAliasesExist(client.admin().indices().prepareAliasesExist("alias-3").get());
544-
545544
}
546545

547546
public void testRestoreTemplates() throws Exception {
@@ -594,7 +593,6 @@ public void testRestoreTemplates() throws Exception {
594593
logger.info("--> check that template is restored");
595594
getIndexTemplatesResponse = client().admin().indices().prepareGetTemplates().get();
596595
assertIndexTemplateExists(getIndexTemplatesResponse, "test-template");
597-
598596
}
599597

600598
public void testIncludeGlobalState() throws Exception {
@@ -781,10 +779,10 @@ public void testIncludeGlobalState() throws Exception {
781779
assertFalse(client().admin().cluster().prepareGetPipeline("barbaz").get().isFound());
782780
assertNull(client().admin().cluster().prepareGetStoredScript("foobar").get().getSource());
783781
assertThat(client.prepareSearch("test-idx").setSize(0).get().getHits().getTotalHits().value, equalTo(100L));
784-
785782
}
786783

787-
public void testSnapshotFileFailureDuringSnapshot() {
784+
public void testSnapshotFileFailureDuringSnapshot() throws InterruptedException {
785+
disableRepoConsistencyCheck("This test uses a purposely broken repository so it would fail consistency checks");
788786
Client client = client();
789787

790788
logger.info("--> creating repository");
@@ -911,6 +909,8 @@ public void testDataFileFailureDuringSnapshot() throws Exception {
911909
}
912910

913911
public void testDataFileFailureDuringRestore() throws Exception {
912+
disableRepoConsistencyCheck("This test intentionally leaves a broken repository");
913+
914914
Path repositoryLocation = randomRepoPath();
915915
Client client = client();
916916
logger.info("--> creating repository");
@@ -974,6 +974,8 @@ public void testDataFileFailureDuringRestore() throws Exception {
974974
}
975975

976976
public void testDataFileCorruptionDuringRestore() throws Exception {
977+
disableRepoConsistencyCheck("This test intentionally leaves a broken repository");
978+
977979
Path repositoryLocation = randomRepoPath();
978980
Client client = client();
979981
logger.info("--> creating repository");
@@ -1238,7 +1240,6 @@ public void testDeletionOfFailingToRecoverIndexShouldStopRestore() throws Except
12381240
assertThat(restoreSnapshotResponse.getRestoreInfo().failedShards(), equalTo(0));
12391241
SearchResponse countResponse = client.prepareSearch("test-idx").setSize(0).get();
12401242
assertThat(countResponse.getHits().getTotalHits().value, equalTo(100L));
1241-
12421243
}
12431244

12441245
public void testUnallocatedShards() throws Exception {
@@ -1703,8 +1704,6 @@ public void testRenameOnRestore() throws Exception {
17031704
.setIndices("test-idx-1").setRenamePattern("test-idx").setRenameReplacement("alias")
17041705
.setWaitForCompletion(true).setIncludeAliases(false).execute().actionGet();
17051706
assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0));
1706-
1707-
17081707
}
17091708

17101709
public void testMoveShardWhileSnapshotting() throws Exception {
@@ -1771,6 +1770,7 @@ public void testMoveShardWhileSnapshotting() throws Exception {
17711770
}
17721771

17731772
public void testDeleteRepositoryWhileSnapshotting() throws Exception {
1773+
disableRepoConsistencyCheck("This test uses a purposely broken repository so it would fail consistency checks");
17741774
Client client = client();
17751775
Path repositoryLocation = randomRepoPath();
17761776
logger.info("--> creating repository");
@@ -2329,7 +2329,6 @@ public void testChangeSettingsOnRestore() throws Exception {
23292329

23302330
assertHitCount(client.prepareSearch("test-idx").setSize(0).setQuery(matchQuery("field1", "Foo")).get(), numdocs);
23312331
assertHitCount(client.prepareSearch("test-idx").setSize(0).setQuery(matchQuery("field1", "bar")).get(), numdocs);
2332-
23332332
}
23342333

23352334
public void testRecreateBlocksOnRestore() throws Exception {
@@ -2423,6 +2422,8 @@ public void testRecreateBlocksOnRestore() throws Exception {
24232422
}
24242423

24252424
public void testCloseOrDeleteIndexDuringSnapshot() throws Exception {
2425+
disableRepoConsistencyCheck("This test intentionally leaves a broken repository");
2426+
24262427
Client client = client();
24272428

24282429
boolean allowPartial = randomBoolean();
@@ -2747,6 +2748,8 @@ private boolean waitForRelocationsToStart(final String index, TimeValue timeout)
27472748
}
27482749

27492750
public void testSnapshotName() throws Exception {
2751+
disableRepoConsistencyCheck("This test does not create any data in the repository");
2752+
27502753
final Client client = client();
27512754

27522755
logger.info("--> creating repository");
@@ -2767,6 +2770,8 @@ public void testSnapshotName() throws Exception {
27672770
}
27682771

27692772
public void testListCorruptedSnapshot() throws Exception {
2773+
disableRepoConsistencyCheck("This test intentionally leaves a broken repository");
2774+
27702775
Client client = client();
27712776
Path repo = randomRepoPath();
27722777
logger.info("--> creating repository at {}", repo.toAbsolutePath());
@@ -3336,6 +3341,9 @@ public void testSnapshotCanceledOnRemovedShard() throws Exception {
33363341
}
33373342

33383343
public void testSnapshotSucceedsAfterSnapshotFailure() throws Exception {
3344+
// TODO: Fix repo cleanup logic to handle these leaked snap-file and only exclude test-repo (the mock repo) here.
3345+
disableRepoConsistencyCheck(
3346+
"This test uses a purposely broken repository implementation that results in leaking snap-{uuid}.dat files");
33393347
logger.info("--> creating repository");
33403348
final Path repoPath = randomRepoPath();
33413349
final Client client = client();

0 commit comments

Comments
 (0)