From c76b596fefdb42793735e7ca423c93807d3ab7d2 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 17 Mar 2021 09:42:33 +0000 Subject: [PATCH 1/2] Avoid atomic overwrite tests on FS repositories Today we leniently permit overwrites of blobs in a repository not to be atomic, since they are not in shared filesystem repositories. In fact it's worse, on Windows overwrites do not even work if there is a concurrent reader. In practice this isn't very important, we do almost no overwrites and almost never read the file that's being overwritten, but we do still test for atomic overwrites in the repository analyzer. With this commit we suppress the atomic overwrite checks in the repository analyzer for FS repositories, and remove the lenience since all other repositories should implement atomic overwrites correctly. Closes #70303 --- .../repositories/blobstore/BlobStoreRepository.java | 9 +++++++++ .../org/elasticsearch/repositories/fs/FsRepository.java | 7 +++++++ .../repositories/encrypted/EncryptedRepository.java | 5 +++++ .../blobstore/testkit/BlobAnalyzeAction.java | 2 -- .../blobstore/testkit/RepositoryAnalyzeAction.java | 5 ++++- 5 files changed, 25 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index 2c71215c135ee..19b9e47c996ce 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -2740,6 +2740,15 @@ public boolean supportURLRepo() { return supportURLRepo; } + /** + * @return whether this repository performs overwrites atomically. In practice we only overwrite the `index.latest` blob so this + * is not very important, but the repository analyzer does test that overwrites happen atomically. It will skip those tests if the + * repository overrides this method to indicate that it does not support atomic overwrites. + */ + public boolean hasAtomicOverwrites() { + return true; + } + /** * The result of removing a snapshot from a shard folder in the repository. */ diff --git a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java index 30b7c935fbb16..746e6726f8ffe 100644 --- a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java @@ -103,4 +103,11 @@ protected BlobStore createBlobStore() throws Exception { protected ByteSizeValue chunkSize() { return chunkSize; } + + @Override + public boolean hasAtomicOverwrites() { + // We overwrite a file by deleting the old file and then renaming the new file into place, which is not atomic. + // Also on Windows the overwrite may fail if the file is opened for reading at the time. + return false; + } } diff --git a/x-pack/plugin/repository-encrypted/src/main/java/org/elasticsearch/repositories/encrypted/EncryptedRepository.java b/x-pack/plugin/repository-encrypted/src/main/java/org/elasticsearch/repositories/encrypted/EncryptedRepository.java index b39a62c7c5e42..7944c20d351e7 100644 --- a/x-pack/plugin/repository-encrypted/src/main/java/org/elasticsearch/repositories/encrypted/EncryptedRepository.java +++ b/x-pack/plugin/repository-encrypted/src/main/java/org/elasticsearch/repositories/encrypted/EncryptedRepository.java @@ -379,6 +379,11 @@ private void validateLocalRepositorySecret(Map snapshotUserMetad } } + @Override + public boolean hasAtomicOverwrites() { + return delegatedRepository.hasAtomicOverwrites(); + } + // pkg-private for tests static final class EncryptedBlobStore implements BlobStore { private final BlobStore delegatedBlobStore; diff --git a/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/BlobAnalyzeAction.java b/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/BlobAnalyzeAction.java index 1bba56bdbd7f6..c975d855178b4 100644 --- a/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/BlobAnalyzeAction.java +++ b/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/BlobAnalyzeAction.java @@ -486,8 +486,6 @@ private void onReadsComplete(Collection responses, WriteDetails wr if (response.isNotFound()) { if (request.readEarly) { nodeFailure = null; // "not found" is legitimate iff we tried to read it before the write completed - } else if (request.writeAndOverwrite) { - nodeFailure = null; // overwrites surprisingly not necessarily atomic, e.g. in a FsBlobContainer } else { nodeFailure = new RepositoryVerificationException( request.getRepositoryName(), diff --git a/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java b/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java index 0e03e2c9f3aa0..6ff3f11e042fc 100644 --- a/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java +++ b/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java @@ -465,7 +465,10 @@ public void run() { request.getReadNodeCount(), request.getEarlyReadNodeCount(), smallBlob && random.nextDouble() < request.getRareActionProbability(), - repository.supportURLRepo() && smallBlob && random.nextDouble() < request.getRareActionProbability() + repository.supportURLRepo() + && repository.hasAtomicOverwrites() + && smallBlob + && random.nextDouble() < request.getRareActionProbability() ) ); queue.add(verifyBlobTask); From 497f50386175dc16d9e3152c9b0433dca3739b5e Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 17 Mar 2021 11:32:15 +0000 Subject: [PATCH 2/2] Spotless --- .../blobstore/testkit/RepositoryAnalyzeAction.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java b/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java index 6ff3f11e042fc..b9b0c057fb5f3 100644 --- a/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java +++ b/x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java @@ -466,9 +466,9 @@ public void run() { request.getEarlyReadNodeCount(), smallBlob && random.nextDouble() < request.getRareActionProbability(), repository.supportURLRepo() - && repository.hasAtomicOverwrites() - && smallBlob - && random.nextDouble() < request.getRareActionProbability() + && repository.hasAtomicOverwrites() + && smallBlob + && random.nextDouble() < request.getRareActionProbability() ) ); queue.add(verifyBlobTask);