Skip to content

Cleanup Stale Root Level Blobs in Sn. Repository #43542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 4, 2019
Merged

Cleanup Stale Root Level Blobs in Sn. Repository #43542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f58957
Cleanup Stale Root Level Blobs in Sn. Repository
original-brownbear Jun 24, 2019
2e12064
Merge remote-tracking branch 'elastic/master' into cleanup-round-2
original-brownbear Jul 3, 2019
75588bb
CR: cleaner blob -> uuid translation + asserts
original-brownbear Jul 3, 2019
778c274
Merge remote-tracking branch 'elastic/master' into cleanup-round-2
original-brownbear Jul 4, 2019
b0c0a65
CR: separately delete current snapshot's root blobs and log at info l…
original-brownbear Jul 4, 2019
915562b
Merge remote-tracking branch 'elastic/master' into cleanup-round-2
original-brownbear Jul 4, 2019
7f2d961
less subtle
original-brownbear Jul 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 54 additions & 3 deletions server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContentFactory;
Expand Down Expand Up @@ -101,6 +102,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -139,7 +141,9 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp

private static final String TESTS_FILE = "tests-";

private static final String METADATA_NAME_FORMAT = "meta-%s.dat";
private static final String METADATA_PREFIX = "meta-";

private static final String METADATA_NAME_FORMAT = METADATA_PREFIX + "%s.dat";

private static final String METADATA_CODEC = "metadata";

Expand Down Expand Up @@ -393,21 +397,24 @@ public void deleteSnapshot(SnapshotId snapshotId, long repositoryStateId, Action
// Delete snapshot from the index file, since it is the maintainer of truth of active snapshots
final RepositoryData updatedRepositoryData;
final Map<String, BlobContainer> foundIndices;
final Set<String> rootBlobs;
try {
final RepositoryData repositoryData = getRepositoryData();
updatedRepositoryData = repositoryData.removeSnapshot(snapshotId);
// Cache the indices that were found before writing out the new index-N blob so that a stuck master will never
// delete an index that was created by another master node after writing this index-N blob.
foundIndices = blobStore().blobContainer(basePath().add("indices")).children();
rootBlobs = blobContainer().listBlobs().keySet();
writeIndexGen(updatedRepositoryData, repositoryStateId);
} catch (Exception ex) {
listener.onFailure(new RepositoryException(metadata.name(), "failed to delete snapshot [" + snapshotId + "]", ex));
return;
}
final SnapshotInfo finalSnapshotInfo = snapshot;
final List<String> snapMetaFilesToDelete =
Arrays.asList(snapshotFormat.blobName(snapshotId.getUUID()), globalMetaDataFormat.blobName(snapshotId.getUUID()));
try {
blobContainer().deleteBlobsIgnoringIfNotExists(
Copy link
Contributor

@ywelsch ywelsch Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we still do this, even though we might clean-up later as well? I wonder if doing this regardless of the listing helps with eventually consistent repos.

Copy link
Contributor

@ywelsch ywelsch Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also explicitly deleting this here will avoid the bogus log message "Found stale root level blobs" later and remove the blobs here from the rootBlobs list

Copy link
Contributor Author

@original-brownbear original-brownbear Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't:

  1. It's highly unlikely that we will miss a blob here even on S3. The only way this would happen in rare cases is if you were to delete a snapshot right after creating it.
  2. It needlessly adds RPC calls (and code) for no apparent gain.
  3. The logging is debug only anyway. If you think that message is confusing, we could just reword the message slightly so that it doesn't look like a potential issue?

Copy link
Contributor

@ywelsch ywelsch Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's highly unlikely that we will miss a blob here even on S3. The only way this would happen in rare cases is if you were to delete a snapshot right after creating it.

True. I think though that if we can avoid relying on list operations for regular clean-up, then we should.

It needlessly adds RPC calls (and code)

It's one extra RPC call per snapshot deletion, so ok I think. It also makes it clearer what needs to be cleaned up as part of normal clean-up operation and what is cleaned up as part of garbage removal. It is a little bit of extra code, but also makes the intent clearer (which files belong to the snapshot and need to be cleaned up). Separating this in terms of code also allows us to apply different logging levels and messages instead of having a vague one. I was pondering for example whether we should provide a summary of clean-up information about dangling snapshot files at info level.

Copy link
Member

@tlrx tlrx Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I think though that if we can avoid relying on list operations for regular clean-up, then we should.

👍

Arrays.asList(snapshotFormat.blobName(snapshotId.getUUID()), globalMetaDataFormat.blobName(snapshotId.getUUID())));
blobContainer().deleteBlobsIgnoringIfNotExists(snapMetaFilesToDelete);
} catch (IOException e) {
logger.warn(() -> new ParameterizedMessage("[{}] Unable to delete global metadata files", snapshotId), e);
}
Expand All @@ -420,12 +427,56 @@ public void deleteSnapshot(SnapshotId snapshotId, long repositoryStateId, Action
snapshotId,
ActionListener.map(listener, v -> {
cleanupStaleIndices(foundIndices, survivingIndices);
cleanupStaleRootFiles(Sets.difference(rootBlobs, new HashSet<>(snapMetaFilesToDelete)), updatedRepositoryData);
return null;
})
);
}
}

private void cleanupStaleRootFiles(Set<String> rootBlobNames, RepositoryData repositoryData) {
final Set<String> allSnapshotIds =
repositoryData.getAllSnapshotIds().stream().map(SnapshotId::getUUID).collect(Collectors.toSet());
final List<String> blobsToDelete = rootBlobNames.stream().filter(
blob -> {
if (FsBlobContainer.isTempBlobName(blob)) {
return true;
}
if (blob.endsWith(".dat")) {
final String foundUUID;
if (blob.startsWith(SNAPSHOT_PREFIX)) {
foundUUID = blob.substring(SNAPSHOT_PREFIX.length(), blob.length() - ".dat".length());
assert snapshotFormat.blobName(foundUUID).equals(blob);
} else if (blob.startsWith(METADATA_PREFIX)) {
foundUUID = blob.substring(METADATA_PREFIX.length(), blob.length() - ".dat".length());
assert globalMetaDataFormat.blobName(foundUUID).equals(blob);
} else {
return false;
}
return allSnapshotIds.contains(foundUUID) == false;
}
return false;
}
).collect(Collectors.toList());
if (blobsToDelete.isEmpty()) {
return;
}
try {
logger.info("[{}] Found stale root level blobs {}. Cleaning them up", metadata.name(), blobsToDelete);
blobContainer().deleteBlobsIgnoringIfNotExists(blobsToDelete);
} catch (IOException e) {
logger.warn(() -> new ParameterizedMessage(
"[{}] The following blobs are no longer part of any snapshot [{}] but failed to remove them",
metadata.name(), blobsToDelete), e);
} catch (Exception e) {
// TODO: We shouldn't be blanket catching and suppressing all exceptions here and instead handle them safely upstream.
// Currently this catch exists as a stop gap solution to tackle unexpected runtime exceptions from implementations
// bubbling up and breaking the snapshot functionality.
assert false : e;
logger.warn(new ParameterizedMessage("[{}] Exception during cleanup of root level blobs", metadata.name()), e);
}
}

private void cleanupStaleIndices(Map<String, BlobContainer> foundIndices, Map<String, IndexId> survivingIndices) {
try {
final Set<String> survivingIndexIds = survivingIndices.values().stream()
Expand Down
7 changes: 7 additions & 0 deletions ...rk/src/main/java/org/elasticsearch/repositories/AbstractThirdPartyRepositoryTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.elasticsearch.threadpool.ThreadPool;

import java.io.ByteArrayInputStream;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -237,6 +238,10 @@ protected void doRun() throws Exception {
final BlobStore blobStore = repo.blobStore();
blobStore.blobContainer(BlobPath.cleanPath().add("indices").add("foo"))
.writeBlob("bar", new ByteArrayInputStream(new byte[0]), 0, false);
for (String prefix : Arrays.asList("snap-", "meta-")) {
blobStore.blobContainer(BlobPath.cleanPath())
.writeBlob(prefix + "foo.dat", new ByteArrayInputStream(new byte[0]), 0, false);
}
future.onResponse(null);
}
});
Expand All @@ -257,6 +262,8 @@ protected void doRun() throws Exception {
future.onResponse(
blobStore.blobContainer(BlobPath.cleanPath().add("indices")).children().containsKey("foo")
&& blobStore.blobContainer(BlobPath.cleanPath().add("indices").add("foo")).blobExists("bar")
&& blobStore.blobContainer(BlobPath.cleanPath()).blobExists("meta-foo.dat")
&& blobStore.blobContainer(BlobPath.cleanPath()).blobExists("snap-foo.dat")
);
}
});
Expand Down