-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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.
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
Improve reaction to blob store corruptions #111954
Merged
DaveCTurner
merged 2 commits into
elastic:main
from
DaveCTurner:2024/08/18/blob-store-corruption-detection
Aug 19, 2024
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
186 changes: 186 additions & 0 deletions
186
...ernalClusterTest/java/org/elasticsearch/repositories/blobstore/BlobStoreCorruptionIT.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,186 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.repositories.blobstore; | ||
|
||
import org.apache.lucene.tests.mockfile.ExtrasFS; | ||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.action.ActionListener; | ||
import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; | ||
import org.elasticsearch.action.support.ActionTestUtils; | ||
import org.elasticsearch.action.support.SubscribableListener; | ||
import org.elasticsearch.action.support.master.AcknowledgedResponse; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.core.CheckedConsumer; | ||
import org.elasticsearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshotsIntegritySuppressor; | ||
import org.elasticsearch.logging.LogManager; | ||
import org.elasticsearch.logging.Logger; | ||
import org.elasticsearch.repositories.fs.FsRepository; | ||
import org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase; | ||
import org.elasticsearch.snapshots.SnapshotState; | ||
import org.elasticsearch.test.ESTestCase; | ||
import org.elasticsearch.test.hamcrest.ElasticsearchAssertions; | ||
import org.junit.Before; | ||
|
||
import java.io.IOException; | ||
import java.nio.file.FileVisitResult; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.nio.file.SimpleFileVisitor; | ||
import java.nio.file.attribute.BasicFileAttributes; | ||
import java.util.ArrayList; | ||
import java.util.Base64; | ||
import java.util.List; | ||
|
||
public class BlobStoreCorruptionIT extends AbstractSnapshotIntegTestCase { | ||
|
||
private static final Logger logger = LogManager.getLogger(BlobStoreCorruptionIT.class); | ||
|
||
@Before | ||
public void suppressConsistencyCheck() { | ||
disableRepoConsistencyCheck("testing corruption detection involves breaking the repo"); | ||
} | ||
|
||
public void testCorruptionDetection() throws Exception { | ||
final var repositoryName = randomIdentifier(); | ||
final var indexName = randomIdentifier(); | ||
final var snapshotName = randomIdentifier(); | ||
final var repositoryRootPath = randomRepoPath(); | ||
|
||
createRepository(repositoryName, FsRepository.TYPE, repositoryRootPath); | ||
createIndexWithRandomDocs(indexName, between(1, 100)); | ||
flushAndRefresh(indexName); | ||
createSnapshot(repositoryName, snapshotName, List.of(indexName)); | ||
|
||
final var corruptedFile = corruptRandomFile(repositoryRootPath); | ||
final var corruptedFileType = RepositoryFileType.getRepositoryFileType(repositoryRootPath, corruptedFile); | ||
final var corruptionDetectors = new ArrayList<CheckedConsumer<ActionListener<Exception>, ?>>(); | ||
|
||
// detect corruption by listing the snapshots | ||
if (corruptedFileType == RepositoryFileType.SNAPSHOT_INFO) { | ||
corruptionDetectors.add(exceptionListener -> { | ||
logger.info("--> listing snapshots"); | ||
client().admin() | ||
.cluster() | ||
.prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repositoryName) | ||
.execute(ActionTestUtils.assertNoSuccessListener(exceptionListener::onResponse)); | ||
}); | ||
} | ||
|
||
// detect corruption by taking another snapshot | ||
if (corruptedFileType == RepositoryFileType.SHARD_GENERATION) { | ||
corruptionDetectors.add(exceptionListener -> { | ||
logger.info("--> taking another snapshot"); | ||
client().admin() | ||
.cluster() | ||
.prepareCreateSnapshot(TEST_REQUEST_TIMEOUT, repositoryName, randomIdentifier()) | ||
.setWaitForCompletion(true) | ||
.execute(exceptionListener.map(createSnapshotResponse -> { | ||
assertNotEquals(SnapshotState.SUCCESS, createSnapshotResponse.getSnapshotInfo().state()); | ||
return new ElasticsearchException("create-snapshot failed as expected"); | ||
})); | ||
}); | ||
} | ||
|
||
// detect corruption by restoring the snapshot | ||
switch (corruptedFileType) { | ||
case SNAPSHOT_INFO, GLOBAL_METADATA, INDEX_METADATA -> corruptionDetectors.add(exceptionListener -> { | ||
logger.info("--> restoring snapshot"); | ||
client().admin() | ||
.cluster() | ||
.prepareRestoreSnapshot(TEST_REQUEST_TIMEOUT, repositoryName, snapshotName) | ||
.setRestoreGlobalState(corruptedFileType == RepositoryFileType.GLOBAL_METADATA || randomBoolean()) | ||
.setWaitForCompletion(true) | ||
.execute(ActionTestUtils.assertNoSuccessListener(exceptionListener::onResponse)); | ||
}); | ||
case SHARD_SNAPSHOT_INFO, SHARD_DATA -> corruptionDetectors.add(exceptionListener -> { | ||
logger.info("--> restoring snapshot and checking for failed shards"); | ||
SubscribableListener | ||
// if shard-level data is corrupted then the overall restore succeeds but the shard recoveries fail | ||
.<AcknowledgedResponse>newForked(l -> client().admin().indices().prepareDelete(indexName).execute(l)) | ||
.andThenAccept(ElasticsearchAssertions::assertAcked) | ||
|
||
.<RestoreSnapshotResponse>andThen( | ||
l -> client().admin() | ||
.cluster() | ||
.prepareRestoreSnapshot(TEST_REQUEST_TIMEOUT, repositoryName, snapshotName) | ||
.setRestoreGlobalState(randomBoolean()) | ||
.setWaitForCompletion(true) | ||
.execute(l) | ||
) | ||
|
||
.addListener(exceptionListener.map(restoreSnapshotResponse -> { | ||
assertNotEquals(0, restoreSnapshotResponse.getRestoreInfo().failedShards()); | ||
return new ElasticsearchException("post-restore recoveries failed as expected"); | ||
})); | ||
}); | ||
} | ||
|
||
try (var ignored = new BlobStoreIndexShardSnapshotsIntegritySuppressor()) { | ||
final var exception = safeAwait(randomFrom(corruptionDetectors)); | ||
logger.info(Strings.format("--> corrupted [%s] and caught exception", corruptedFile), exception); | ||
} | ||
} | ||
|
||
private static Path corruptRandomFile(Path repositoryRootPath) throws IOException { | ||
final var corruptedFileType = getRandomCorruptibleFileType(); | ||
final var corruptedFile = getRandomFileToCorrupt(repositoryRootPath, corruptedFileType); | ||
if (randomBoolean()) { | ||
logger.info("--> deleting [{}]", corruptedFile); | ||
Files.delete(corruptedFile); | ||
} else { | ||
corruptFileContents(corruptedFile); | ||
} | ||
return corruptedFile; | ||
} | ||
|
||
private static void corruptFileContents(Path fileToCorrupt) throws IOException { | ||
final var oldFileContents = Files.readAllBytes(fileToCorrupt); | ||
logger.info("--> contents of [{}] before corruption: [{}]", fileToCorrupt, Base64.getEncoder().encodeToString(oldFileContents)); | ||
final byte[] newFileContents = new byte[randomBoolean() ? oldFileContents.length : between(0, oldFileContents.length)]; | ||
System.arraycopy(oldFileContents, 0, newFileContents, 0, newFileContents.length); | ||
if (newFileContents.length == oldFileContents.length) { | ||
final var corruptionPosition = between(0, newFileContents.length - 1); | ||
newFileContents[corruptionPosition] = randomValueOtherThan(oldFileContents[corruptionPosition], ESTestCase::randomByte); | ||
logger.info( | ||
"--> updating byte at position [{}] from [{}] to [{}]", | ||
corruptionPosition, | ||
oldFileContents[corruptionPosition], | ||
newFileContents[corruptionPosition] | ||
); | ||
} else { | ||
logger.info("--> truncating file from length [{}] to length [{}]", oldFileContents.length, newFileContents.length); | ||
} | ||
Files.write(fileToCorrupt, newFileContents); | ||
logger.info("--> contents of [{}] after corruption: [{}]", fileToCorrupt, Base64.getEncoder().encodeToString(newFileContents)); | ||
} | ||
|
||
private static RepositoryFileType getRandomCorruptibleFileType() { | ||
return randomValueOtherThanMany( | ||
// these blob types do not have reliable corruption detection, so we must skip them | ||
t -> t == RepositoryFileType.ROOT_INDEX_N || t == RepositoryFileType.ROOT_INDEX_LATEST, | ||
() -> randomFrom(RepositoryFileType.values()) | ||
); | ||
} | ||
|
||
private static Path getRandomFileToCorrupt(Path repositoryRootPath, RepositoryFileType corruptedFileType) throws IOException { | ||
final var corruptibleFiles = new ArrayList<Path>(); | ||
Files.walkFileTree(repositoryRootPath, new SimpleFileVisitor<>() { | ||
@Override | ||
public FileVisitResult visitFile(Path filePath, BasicFileAttributes attrs) throws IOException { | ||
if (ExtrasFS.isExtra(filePath.getFileName().toString()) == false | ||
&& RepositoryFileType.getRepositoryFileType(repositoryRootPath, filePath) == corruptedFileType) { | ||
corruptibleFiles.add(filePath); | ||
} | ||
return super.visitFile(filePath, attrs); | ||
} | ||
}); | ||
return randomFrom(corruptibleFiles); | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
27 changes: 27 additions & 0 deletions
27
...sticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshotsIntegritySuppressor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.index.snapshots.blobstore; | ||
|
||
import org.elasticsearch.core.Releasable; | ||
|
||
/** | ||
* Test utility class to suppress assertions about the integrity of the contents of a blobstore repository, in order to verify the | ||
* production behaviour on encountering invalid data. | ||
*/ | ||
public class BlobStoreIndexShardSnapshotsIntegritySuppressor implements Releasable { | ||
|
||
public BlobStoreIndexShardSnapshotsIntegritySuppressor() { | ||
BlobStoreIndexShardSnapshots.INTEGRITY_ASSERTIONS_ENABLED = false; | ||
} | ||
|
||
@Override | ||
public void close() { | ||
BlobStoreIndexShardSnapshots.INTEGRITY_ASSERTIONS_ENABLED = true; | ||
} | ||
} |
60 changes: 60 additions & 0 deletions
60
.../framework/src/main/java/org/elasticsearch/repositories/blobstore/RepositoryFileType.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.repositories.blobstore; | ||
|
||
import org.elasticsearch.common.Strings; | ||
|
||
import java.nio.file.Path; | ||
import java.util.regex.Pattern; | ||
|
||
/** | ||
* The types of blobs in a {@link BlobStoreRepository}. | ||
*/ | ||
public enum RepositoryFileType { | ||
|
||
ROOT_INDEX_N("index-NUM"), | ||
ROOT_INDEX_LATEST("index.latest"), | ||
SNAPSHOT_INFO("snap-UUID.dat"), | ||
GLOBAL_METADATA("meta-UUID.dat"), | ||
INDEX_METADATA("indices/UUID/meta-SHORTUUID.dat"), | ||
SHARD_GENERATION("indices/UUID/NUM/index-UUID"), | ||
SHARD_SNAPSHOT_INFO("indices/UUID/NUM/snap-UUID.dat"), | ||
SHARD_DATA("indices/UUID/NUM/__UUID"), | ||
// NB no support for legacy names (yet) | ||
; | ||
|
||
private final Pattern pattern; | ||
|
||
RepositoryFileType(String regex) { | ||
pattern = Pattern.compile( | ||
"^(" | ||
+ regex | ||
// decimal numbers | ||
.replace("NUM", "(0|[1-9][0-9]*)") | ||
// 15-byte UUIDS from TimeBasedUUIDGenerator | ||
.replace("SHORTUUID", "[0-9a-zA-Z_-]{20}") | ||
// 16-byte UUIDs from RandomBasedUUIDGenerator | ||
.replace("UUID", "[0-9a-zA-Z_-]{22}") | ||
+ ")$" | ||
); | ||
} | ||
|
||
public static RepositoryFileType getRepositoryFileType(Path repositoryRoot, Path blobPath) { | ||
final var relativePath = repositoryRoot.relativize(blobPath).toString().replace(repositoryRoot.getFileSystem().getSeparator(), "/"); | ||
for (final var repositoryFileType : RepositoryFileType.values()) { | ||
if (repositoryFileType.pattern.matcher(relativePath).matches()) { | ||
return repositoryFileType; | ||
} | ||
} | ||
throw new IllegalArgumentException( | ||
Strings.format("[%s] is not the path of a known blob type within [%s]", relativePath, repositoryRoot) | ||
); | ||
} | ||
|
||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this potentially be considered an example of use randomised testing for coverage?
It seems we've enumerated the ways in which we expect to detect each type of corruption, then we're randomly picking one to execute. Would it not be better to test them all? or does the execution of one have side-effects precluding the others running?
More a question than a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ehh kinda, tho the coverage is extremely low even if we did check all these paths on each test run. The assertions we were previously hitting would only trip if you corrupted a very specific byte in exactly the right way.
Really this is just to verify that a corruption doesn't trip assertions so that we can proceed with #93735 that will introduce a way to catch all these problems at once.