Snapshot UUIDs in blob names #19421

abeyad · 2016-07-13T18:18:12Z

This PR adds to #18815 to enable safer behavior with respect to snapshots. #18815 makes blob deletions strict - if the blob doesn't exist, an exception is thrown instead of silently failing. However, this presents an issue with some of our tests scenarios (and possibly real life occurrences) where the inability to delete a blob because it was deleted by someone else would cause the snapshot deletion itself to fail, potentially leaving other blobs around. This could even happen if the snap-.dat blob was successfully deleted but the machine crashed before the other blobs in the snapshot could be deleted. Our current mechanism uses a listing of the snap-.dat blobs to determine what the current snapshots are. If deletions can't be relied upon, then we can't be sure that the existence of snap-A.dat in the repository implies that A is a current snapshot.

This PR uses the index generational files from #19002 to retrieve the snapshot UUID for snapshots and name all blobs by the snapshot UUID instead of the snapshot name. If a snapshot A was deleted, then recreated, but not all of A's files were deleted, then we would have to worry about overwriting existing blobs, which is problematic. By naming blobs with the snapshot UUID, we avoid this issue.

This PR also introduces a unique index ID (a UUID) for indices in the snapshots, so index folders can be named by the UUID and avoid problems such as #7540.

Relates #18156

abeyad · 2016-07-14T00:43:18Z

@imotov your review would be most appreciated

@gfyoung the elastic:enhancement/snapshot-blob-handling contains your commits and this PR is against that branch

imotov · 2016-07-19T19:22:12Z

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobContainer.java

@@ -72,14 +72,14 @@ public InputStream readBlob(String blobName) throws IOException {
        logger.trace("readBlob({})", blobName);

        if (!blobExists(blobName)) {


I know that this is a part of another PR, but since we are modifying it here - why did we need it in the first place? It looks like we are handling file not found condition when we try to read from the stream anyway. Why do it twice?

@imotov: Azure is a little too lenient about this unfortunately (at least in testing), hence necessitating this check. FWIW, @abeyad could remove it and run the unit tests again that I wrote to see whether we still get those failures.

@gfyoung what do you mean by lenient? What does it do if the file doesn't exist and you are trying to read the file?

@imotov : What I meant was that it didn't throw Exceptions during testing (e.g. when I tried deleting a blob that didn't exist). Of course, this was using a mock, so I don't know if that would be the case in real life, but testing purposes, the existence check was necessary for it to pass.

The better strategy is to change the mocks to conform to how the real APIs would behave (i.e. throw a 404 if the blob doesn't exist), in which case, the blobExists check is extraneous. I will take care of this in a separate commit as part of the PR

imotov · 2016-07-19T20:31:59Z

@abeyad Looks good. I left a few minor comments.

Makes deleting snapshots more robust by first deleting the snapshot from the index generational file, then handling individual deletion file errors with log messages instead of failing the entire operation.

for reads and deletes if the blob does not exist.

to snapshot/restore) and the index to UUID mapping is stored in the repository index file.

Azure and Google cloud blob containers, as the APIs for both return a 404 in the case of a missing object, which we already handle through a NoSuchFileFoundException.

abeyad · 2016-07-24T19:09:51Z

@imotov I've pushed a6f5e0b to address code review comments and 299b8a7 to remove the blobExists check from readBlob methods

imotov · 2016-07-27T20:27:07Z

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

@@ -223,6 +228,9 @@

    private final ChecksumBlobStoreFormat<BlobStoreIndexShardSnapshots> indexShardSnapshotsFormat;

+    // flag to indicate if the index gen file has been checked for updating from pre 5.0 versions
+    private volatile boolean indexGenChecked;


I have a feeling that this might cause some issues. We don't control the underlying storage, which might change on us without our knowledge. I would like to run a couple of scenarios by you when you have a chance to see if we should/can make it more robust.

imotov · 2016-07-27T21:36:47Z

@abeyad left a couple of comments

upgrades if it determines the read data is in the legacy format. It writes the upgraded version if it is not a read-only repository and caches the repository data if it is a read-only repository.

abeyad · 2016-07-29T02:11:24Z

@imotov i pushed 58d6b9d

imotov · 2016-07-30T19:04:19Z

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

-            RepositoryData repositoryData = updateIndexGenIfNecessary();
-            if (repositoryData == null) {
-                repositoryData = readIndexGen();
+            RepositoryData repositoryData = readIndexGen();


I think if you collapse readIndexGen into getRepositoryData you will be able to remove otherwise unnecessary isLegacyFormat flag from RepositoryData.

Good catch @imotov, I'll push a commit with those changes

imotov · 2016-07-30T19:04:55Z

Left a minor comment. Otherwise, LGTM.

abeyad · 2016-07-31T04:04:43Z

@imotov I pushed 0f335ac

abeyad · 2016-07-31T04:18:46Z

@imotov thanks for the review!

abeyad added >enhancement WIP :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs :Internal v5.0.0-alpha5 labels Jul 13, 2016

abeyad self-assigned this Jul 13, 2016

abeyad force-pushed the snapshot-uuids-in-blob-names branch from 045a386 to 4a0ad6e Compare July 14, 2016 00:41

abeyad removed the WIP label Jul 14, 2016

abeyad changed the title ~~[WIP] Snapshot UUIDs in blob names~~ Snapshot UUIDs in blob names Jul 14, 2016

imotov reviewed Jul 19, 2016
View reviewed changes

abeyad force-pushed the enhancement/snapshot-blob-handling branch from c8f1258 to 6a9f488 Compare July 22, 2016 17:49

Ali Beyad added 4 commits July 22, 2016 13:49

More robust handling of snapshot deletions

abaf844

Makes deleting snapshots more robust by first deleting the snapshot from the index generational file, then handling individual deletion file errors with log messages instead of failing the entire operation.

Change the BlobContainer interface to throw a NoSuchFileFoundException

630218a

for reads and deletes if the blob does not exist.

All snapshot metadata files use UUID for the blob ID

a0a4d67

Index folder names now use a UUID (not the index UUID but one specific

d9ec959

to snapshot/restore) and the index to UUID mapping is stored in the repository index file.

abeyad force-pushed the snapshot-uuids-in-blob-names branch from a219c70 to f0c9f6e Compare July 23, 2016 20:20

Ali Beyad added 2 commits July 23, 2016 23:24

Remove IndexMeta and addresses code review comments

a6f5e0b

Removes unnecessary blobExists() check before reading a blob in the

299b8a7

Azure and Google cloud blob containers, as the APIs for both return a 404 in the case of a missing object, which we already handle through a NoSuchFileFoundException.

abeyad force-pushed the snapshot-uuids-in-blob-names branch from f0c9f6e to 299b8a7 Compare July 24, 2016 19:08

imotov reviewed Jul 27, 2016
View reviewed changes

clintongormley added v5.0.0-beta1 and removed v5.0.0-alpha5 labels Jul 28, 2016

This commit first reads the repository data and only

58d6b9d

upgrades if it determines the read data is in the legacy format. It writes the upgraded version if it is not a read-only repository and caches the repository data if it is a read-only repository.

abeyad force-pushed the snapshot-uuids-in-blob-names branch from 0148175 to 58d6b9d Compare July 29, 2016 02:09

imotov reviewed Jul 30, 2016
View reviewed changes

Removes legacy format in RepositoryData

0f335ac

abeyad merged commit ce88815 into elastic:enhancement/snapshot-blob-handling Jul 31, 2016

This was referenced Jul 31, 2016

Use UUIDs in working with snapshots #18156

Closed

More resilient blob handling in snapshot repositories #19706

Merged

clintongormley added v5.0.0-alpha5 and removed v5.0.0-beta1 labels Aug 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshot UUIDs in blob names #19421

Snapshot UUIDs in blob names #19421

abeyad commented Jul 13, 2016

abeyad commented Jul 14, 2016

imotov Jul 19, 2016

gfyoung Jul 20, 2016 •

edited

Loading

imotov Jul 21, 2016

gfyoung Jul 21, 2016

abeyad Jul 22, 2016

imotov commented Jul 19, 2016

abeyad commented Jul 24, 2016

imotov Jul 27, 2016

imotov commented Jul 27, 2016

abeyad commented Jul 29, 2016

imotov Jul 30, 2016

abeyad Jul 30, 2016

imotov commented Jul 30, 2016

abeyad commented Jul 31, 2016

abeyad commented Jul 31, 2016

		@@ -72,14 +72,14 @@ public InputStream readBlob(String blobName) throws IOException {
		logger.trace("readBlob({})", blobName);

		if (!blobExists(blobName)) {

Snapshot UUIDs in blob names #19421

Snapshot UUIDs in blob names #19421

Conversation

abeyad commented Jul 13, 2016

abeyad commented Jul 14, 2016

imotov Jul 19, 2016

Choose a reason for hiding this comment

gfyoung Jul 20, 2016 • edited Loading

Choose a reason for hiding this comment

imotov Jul 21, 2016

Choose a reason for hiding this comment

gfyoung Jul 21, 2016

Choose a reason for hiding this comment

abeyad Jul 22, 2016

Choose a reason for hiding this comment

imotov commented Jul 19, 2016

abeyad commented Jul 24, 2016

imotov Jul 27, 2016

Choose a reason for hiding this comment

imotov commented Jul 27, 2016

abeyad commented Jul 29, 2016

imotov Jul 30, 2016

Choose a reason for hiding this comment

abeyad Jul 30, 2016

Choose a reason for hiding this comment

imotov commented Jul 30, 2016

abeyad commented Jul 31, 2016

abeyad commented Jul 31, 2016

gfyoung Jul 20, 2016 •

edited

Loading