From 3dff0d0de209437d727a0f309111f5ed49ebfb2e Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Fri, 3 Mar 2017 17:01:51 -0500 Subject: [PATCH] Azure blob store's readBlob() method first checks if the blob exists (#23483) Previously, the Azure blob store would depend on a 404 StorageException coming back from Azure if trying to open an input stream to a non-existent blob. This works for Azure repositories which access a primary location path. For those configured to access a secondary location path, the Azure SDK keeps trying for a long while before returning a 404 StorageException, causing potential delays in the snapshot APIs. This commit makes an initial check if the blob exists in Azure and returns immediately with a NoSuchFileException, instead of trying to open the input stream to the blob. Closes #23480 --- .../azure/blobstore/AzureBlobContainer.java | 11 ++ .../cloud/azure/blobstore/AzureBlobStore.java | 7 ++ ...zureSnapshotRestoreListSnapshotsTests.java | 117 ++++++++++++++++++ .../azure/AzureSnapshotRestoreTests.java | 2 +- 4 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreListSnapshotsTests.java diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobContainer.java b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobContainer.java index 66671931683a..b1b359956b6d 100644 --- a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobContainer.java +++ b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobContainer.java @@ -19,6 +19,7 @@ package org.elasticsearch.cloud.azure.blobstore; +import com.microsoft.azure.storage.LocationMode; import com.microsoft.azure.storage.StorageException; import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Nullable; @@ -68,6 +69,16 @@ public class AzureBlobContainer extends AbstractBlobContainer { public InputStream readBlob(String blobName) throws IOException { logger.trace("readBlob({})", blobName); + if (blobStore.getLocationMode() == LocationMode.SECONDARY_ONLY && !blobExists(blobName)) { + // On Azure, if the location path is a secondary location, and the blob does not + // exist, instead of returning immediately from the getInputStream call below + // with a 404 StorageException, Azure keeps trying and trying for a long timeout + // before throwing a storage exception. This can cause long delays in retrieving + // snapshots, so we first check if the blob exists before trying to open an input + // stream to it. + throw new NoSuchFileException("Blob [" + blobName + "] does not exist"); + } + try { return blobStore.getInputStream(blobStore.container(), buildKey(blobName)); } catch (StorageException e) { diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobStore.java b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobStore.java index e7b6911447f1..73b0f07835ce 100644 --- a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobStore.java +++ b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobStore.java @@ -76,6 +76,13 @@ public class AzureBlobStore extends AbstractComponent implements BlobStore { return container; } + /** + * Gets the configured {@link LocationMode} for the Azure storage requests. + */ + public LocationMode getLocationMode() { + return locMode; + } + @Override public BlobContainer blobContainer(BlobPath path) { return new AzureBlobContainer(repositoryName, path, this); diff --git a/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreListSnapshotsTests.java b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreListSnapshotsTests.java new file mode 100644 index 000000000000..e05911cd1e93 --- /dev/null +++ b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreListSnapshotsTests.java @@ -0,0 +1,117 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.repositories.azure; + +import com.microsoft.azure.storage.LocationMode; +import com.microsoft.azure.storage.StorageException; +import org.elasticsearch.action.admin.cluster.repositories.put.PutRepositoryResponse; +import org.elasticsearch.client.Client; +import org.elasticsearch.cloud.azure.AbstractAzureWithThirdPartyIntegTestCase; +import org.elasticsearch.cloud.azure.storage.AzureStorageService; +import org.elasticsearch.cloud.azure.storage.AzureStorageServiceImpl; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.repositories.azure.AzureRepository.Repository; +import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.test.ESIntegTestCase.ClusterScope; +import org.junit.After; +import org.junit.Before; + +import java.net.URISyntaxException; +import java.util.concurrent.TimeUnit; + +import static org.elasticsearch.cloud.azure.AzureTestUtils.readSettingsFromFile; +import static org.elasticsearch.repositories.azure.AzureSnapshotRestoreTests.getContainerName; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.lessThanOrEqualTo; + +/** + * This test needs Azure to run and -Dtests.thirdparty=true to be set + * and -Dtests.config=/path/to/elasticsearch.yml + * + * Note that this test requires an Azure storage account, with the account + * and credentials set in the elasticsearch.yml config file passed in to the + * test. The Azure storage account type must be a Read-access geo-redundant + * storage (RA-GRS) account. + * + * @see AbstractAzureWithThirdPartyIntegTestCase + */ +@ClusterScope( + scope = ESIntegTestCase.Scope.SUITE, + supportsDedicatedMasters = false, numDataNodes = 1, + transportClientRatio = 0.0) +public class AzureSnapshotRestoreListSnapshotsTests extends AbstractAzureWithThirdPartyIntegTestCase { + + private final AzureStorageService azureStorageService = new AzureStorageServiceImpl(readSettingsFromFile()); + private final String containerName = getContainerName(); + + public void testList() throws Exception { + Client client = client(); + logger.info("--> creating azure primary repository"); + PutRepositoryResponse putRepositoryResponsePrimary = client.admin().cluster().preparePutRepository("primary") + .setType("azure").setSettings(Settings.builder() + .put(Repository.CONTAINER_SETTING.getKey(), containerName) + ).get(); + assertThat(putRepositoryResponsePrimary.isAcknowledged(), equalTo(true)); + + logger.info("--> start get snapshots on primary"); + long startWait = System.currentTimeMillis(); + client.admin().cluster().prepareGetSnapshots("primary").get(); + long endWait = System.currentTimeMillis(); + // definitely should be done in 30s, and if its not working as expected, it takes over 1m + assertThat(endWait - startWait, lessThanOrEqualTo(30000L)); + + logger.info("--> creating azure secondary repository"); + PutRepositoryResponse putRepositoryResponseSecondary = client.admin().cluster().preparePutRepository("secondary") + .setType("azure").setSettings(Settings.builder() + .put(Repository.CONTAINER_SETTING.getKey(), containerName) + .put(Repository.LOCATION_MODE_SETTING.getKey(), "secondary_only") + ).get(); + assertThat(putRepositoryResponseSecondary.isAcknowledged(), equalTo(true)); + + logger.info("--> start get snapshots on secondary"); + startWait = System.currentTimeMillis(); + client.admin().cluster().prepareGetSnapshots("secondary").get(); + endWait = System.currentTimeMillis(); + logger.info("--> end of get snapshots on secondary. Took {} ms", endWait - startWait); + assertThat(endWait - startWait, lessThanOrEqualTo(30000L)); + } + + @Before + public void createContainer() throws Exception { + // It could happen that we run this test really close to a previous one + // so we might need some time to be able to create the container + assertBusy(() -> { + try { + azureStorageService.createContainer(null, LocationMode.PRIMARY_ONLY, containerName); + } catch (URISyntaxException e) { + // Incorrect URL. This should never happen. + fail(); + } catch (StorageException e) { + // It could happen. Let's wait for a while. + fail(); + } + }, 30, TimeUnit.SECONDS); + } + + @After + public void removeContainer() throws Exception { + azureStorageService.removeContainer(null, LocationMode.PRIMARY_ONLY, containerName); + } +} diff --git a/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java index d149cc9d2015..fe4d458cb651 100644 --- a/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java +++ b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java @@ -69,7 +69,7 @@ public class AzureSnapshotRestoreTests extends AbstractAzureWithThirdPartyIntegT return testName.contains(" ") ? Strings.split(testName, " ")[0] : testName; } - private static String getContainerName() { + public static String getContainerName() { String testName = "snapshot-itest-".concat(RandomizedTest.getContext().getRunnerSeedAsString().toLowerCase(Locale.ROOT)); return testName.contains(" ") ? Strings.split(testName, " ")[0] : testName; }