SNAPSHOT+TESTS: Rem. Mock Atomic Writes Randomness

[original-brownbear]

• Randomly doing non-atomic writes in place of atomic ones causes rare 0 byte reads from index-N files in tests (SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard failing on Master #37005 (comment))
• Removing this randomness fixes these random failures and is valid because it does not reproduce a real-world failure-mode:
• Cloud based Blob stores (S3, GCS and Azure) do not have inconsistent partial reads of a blob, either you read a complete blob or nothing on them
• For file system based blob stores the atomic move we do (to atomically write a file) by setting java.nio.file.StandardCopyOption#ATOMIC_MOVE would throw if the file system does not provide for atomic moves
• Closes SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard failing on Master #37005

[elasticmachine]

Pinging @elastic/es-distributed

[ywelsch]

yes, it's not an issue for the repository plugins that we provide, but the BlobContainer interface allows for this (see Javadocs of BlobContainer#writeBlobAtomic). I suggest we change the spec in this regard for 7.0 (requiring atomic writes when called by this method), and only then proceed with this change here.

[original-brownbear]

I suggest we change the spec in this regard for 7.0 (requiring atomic writes when called by this method), and only then proceed with this change here.

Should we fix the production code in 6.x then to fix these tests? We could simply break out of the read logic for repository data here https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java#L670 and return empty data if we read an empty blob (i.e. treat an empty blob like a missing file)?

[original-brownbear]

@ywelsch as a matter of fact, I think org.elasticsearch.repositories.hdfs.HdfsBlobContainer actually is an implementation that isn't atomic (missed that one when I opened the PR) in its writes. We could make it atomic though by doing the same thing we do for the fs repository (write to tempfile -> then move)?

=> Fix hdfs repository, adjust docs and just target 7.0 here?

[ywelsch]

The problem with moving HDFS to the "write temp file -> atomic rename" pattern is that it will make writing to an HDFS repository slower (more metadata requests, which are executed sequentially on the namenode). My prior experience with HDFS showed that these operations can possibly take upward of a few 100 milliseconds to complete. This might be an ok tradeoff though if we can simplify the spec in ES 7. We still have to decide what to do for 6.x. Is there an alternative to removing the allow_atomic_operations on 6.x?

[original-brownbear]

@ywelsch

Is there an alternative to removing the allow_atomic_operations on 6.x?

I suggested one above #37011 (comment) (simply break out of the read on empty blob and treat them like missing blobs). WDYT?

... that may also be an alternative for 7.0 if we don't want to make the trade-off, but it seems to me like there are possible production issues with this method not being consistent?

[ywelsch]

simply break out of the read on empty blob and treat them like missing blobs

what if the file is not empty, but partially written?

Fix hdfs repository, adjust docs

Let's do this for both 7.0 and 6.x

[original-brownbear]

@ywelsch

what if the file is not empty, but partially written?

I don't think this will happen with the file system store because the content of that file is small enough to be less than a sector and just sync atomically right?
It seems to me handling the partially written situation is overthinking the problem potentially. In the end this isn't going to happen in the real world with any of the cloud blob stores, HDFS (your block size is always going to be more than the size of this file there) or the fs repository as pointed above.

=> Is there really a practical situation where we want to support blob repositories that need multi-part writes for this little data?

[ywelsch]

I wouldn't want to make any strong assumptions about the size of the files that are to be written with writeAtomic. Let's make HDFS atomic and proceed with this change here, both on 6.x and 7.0

[original-brownbear]

@ywelsch

I suggest we change the spec in this regard for 7.0 (requiring atomic writes when called by this method), and only then proceed with this change here.

I guess it's not only changing docs here right? Basically we'd change the interface to require implementation of org.elasticsearch.common.blobstore.BlobContainer#writeBlobAtomic and remove the default fallback to writeBlob?

[ywelsch]

I guess it's not only changing docs here right? Basically we'd change the interface to require implementation of org.elasticsearch.common.blobstore.BlobContainer#writeBlobAtomic and remove the default fallback to writeBlob?

yes, we can restrict that change to 7.0 though.

[original-brownbear]

@ywelsch with HDFS now fixed we could change the interface to not do the default fallback to non-atomic write for master/7.x.

But do we even want to do anything else in this PR (since it needs to fix master and 6.x)?

The docs there simply state:

        When the BlobContainer implementation
     * does not provide a specific implementation of writeBlobAtomic(String, InputStream, long), then
     * the {@link #writeBlob(String, InputStream, long, boolean)} method is used.

This is factually correct and we don't want to change this for 6.x, => what would we even change in the docs in this PR?

[original-brownbear]

@ywelsch thanks, will merge once green. The new commit I just pushed is just re-enabling the test fixed here.