Skip to content

Commit ba72b64

Browse files
committed
Avoid atomic overwrite tests on FS repositories (#70483)
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
1 parent 1b74810 commit ba72b64

File tree

5 files changed

+25
-3
lines changed

5 files changed

+25
-3
lines changed

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2771,6 +2771,15 @@ public boolean supportURLRepo() {
27712771
return supportURLRepo;
27722772
}
27732773

2774+
/**
2775+
* @return whether this repository performs overwrites atomically. In practice we only overwrite the `index.latest` blob so this
2776+
* is not very important, but the repository analyzer does test that overwrites happen atomically. It will skip those tests if the
2777+
* repository overrides this method to indicate that it does not support atomic overwrites.
2778+
*/
2779+
public boolean hasAtomicOverwrites() {
2780+
return true;
2781+
}
2782+
27742783
/**
27752784
* The result of removing a snapshot from a shard folder in the repository.
27762785
*/

server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,4 +119,11 @@ protected ByteSizeValue chunkSize() {
119119
public BlobPath basePath() {
120120
return basePath;
121121
}
122+
123+
@Override
124+
public boolean hasAtomicOverwrites() {
125+
// We overwrite a file by deleting the old file and then renaming the new file into place, which is not atomic.
126+
// Also on Windows the overwrite may fail if the file is opened for reading at the time.
127+
return false;
128+
}
122129
}

x-pack/plugin/repository-encrypted/src/main/java/org/elasticsearch/repositories/encrypted/EncryptedRepository.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,11 @@ private void validateLocalRepositorySecret(Map<String, Object> snapshotUserMetad
385385
}
386386
}
387387

388+
@Override
389+
public boolean hasAtomicOverwrites() {
390+
return delegatedRepository.hasAtomicOverwrites();
391+
}
392+
388393
// pkg-private for tests
389394
static final class EncryptedBlobStore implements BlobStore {
390395
private final BlobStore delegatedBlobStore;

x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/BlobAnalyzeAction.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -486,8 +486,6 @@ private void onReadsComplete(Collection<NodeResponse> responses, WriteDetails wr
486486
if (response.isNotFound()) {
487487
if (request.readEarly) {
488488
nodeFailure = null; // "not found" is legitimate iff we tried to read it before the write completed
489-
} else if (request.writeAndOverwrite) {
490-
nodeFailure = null; // overwrites surprisingly not necessarily atomic, e.g. in a FsBlobContainer
491489
} else {
492490
nodeFailure = new RepositoryVerificationException(
493491
request.getRepositoryName(),

x-pack/plugin/snapshot-repo-test-kit/src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,10 @@ public void run() {
465465
request.getReadNodeCount(),
466466
request.getEarlyReadNodeCount(),
467467
smallBlob && random.nextDouble() < request.getRareActionProbability(),
468-
repository.supportURLRepo() && smallBlob && random.nextDouble() < request.getRareActionProbability()
468+
repository.supportURLRepo()
469+
&& repository.hasAtomicOverwrites()
470+
&& smallBlob
471+
&& random.nextDouble() < request.getRareActionProbability()
469472
)
470473
);
471474
queue.add(verifyBlobTask);

0 commit comments

Comments
 (0)