From 87c1ec37c78a8c45b2d98f6e1257aa90c688b4c9 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 15 Dec 2021 20:47:59 -0500 Subject: [PATCH 1/2] [plugin] repository-azure is not working properly hangs on basic operations Signed-off-by: Andriy Redko --- .../repositories/azure/AzureBlobStore.java | 174 ++++++++++++------ 1 file changed, 114 insertions(+), 60 deletions(-) diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java index d4f3acf0a5c66..022b81b990e04 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java @@ -35,6 +35,7 @@ import com.azure.core.http.HttpMethod; import com.azure.core.http.HttpRequest; import com.azure.core.http.HttpResponse; +import com.azure.core.http.rest.PagedResponse; import com.azure.core.http.rest.Response; import com.azure.core.util.Context; import com.azure.storage.blob.BlobClient; @@ -51,6 +52,7 @@ import com.azure.storage.blob.options.BlobParallelUploadOptions; import com.azure.storage.common.implementation.Constants; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.core.util.Throwables; @@ -82,6 +84,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicLong; @@ -217,50 +220,69 @@ public DeleteResult deleteBlobDirectory(String path, Executor executor) throws U final ListBlobsOptions listBlobsOptions = new ListBlobsOptions().setPrefix(path); SocketAccess.doPrivilegedVoidException(() -> { - for (final BlobItem blobItem : blobContainer.listBlobs(listBlobsOptions, timeout())) { - // Skipping prefixes as those are not deletable and should not be there - assert (blobItem.isPrefix() == null || !blobItem.isPrefix()) : "Only blobs (not prefixes) are expected"; - - outstanding.incrementAndGet(); - executor.execute(new AbstractRunnable() { - @Override - protected void doRun() throws Exception { - final long len = blobItem.getProperties().getContentLength(); - - final BlobClient azureBlob = blobContainer.getBlobClient(blobItem.getName()); - logger.trace( - () -> new ParameterizedMessage("container [{}]: blob [{}] found. removing.", container, blobItem.getName()) - ); - final Response response = azureBlob.deleteWithResponse(null, null, timeout(), client.v2().get()); - logger.trace( - () -> new ParameterizedMessage( - "container [{}]: blob [{}] deleted status [{}].", - container, - blobItem.getName(), - response.getStatusCode() - ) - ); - - blobsDeleted.incrementAndGet(); - if (len >= 0) { - bytesDeleted.addAndGet(len); + String continuationToken = null; + + do { + // Fetch one page at a time, others are going to be fetched by continuation token + final Optional> pageOpt = blobContainer.listBlobs(listBlobsOptions, timeout()) + .streamByPage(continuationToken) + .findFirst(); + + if (!pageOpt.isPresent()) { + // No more pages, should never happen + break; + } + + final PagedResponse page = pageOpt.get(); + for (final BlobItem blobItem : page.getValue()) { + // Skipping prefixes as those are not deletable and should not be there + assert (blobItem.isPrefix() == null || !blobItem.isPrefix()) : "Only blobs (not prefixes) are expected"; + + outstanding.incrementAndGet(); + executor.execute(new AbstractRunnable() { + @Override + protected void doRun() throws Exception { + final long len = blobItem.getProperties().getContentLength(); + + final BlobClient azureBlob = blobContainer.getBlobClient(blobItem.getName()); + logger.trace( + () -> new ParameterizedMessage("container [{}]: blob [{}] found. removing.", container, blobItem.getName()) + ); + final Response response = azureBlob.deleteWithResponse(null, null, timeout(), client.v2().get()); + logger.trace( + () -> new ParameterizedMessage( + "container [{}]: blob [{}] deleted status [{}].", + container, + blobItem.getName(), + response.getStatusCode() + ) + ); + + blobsDeleted.incrementAndGet(); + if (len >= 0) { + bytesDeleted.addAndGet(len); + } } - } - @Override - public void onFailure(Exception e) { - exceptions.add(e); - } + @Override + public void onFailure(Exception e) { + exceptions.add(e); + } - @Override - public void onAfter() { - if (outstanding.decrementAndGet() == 0) { - result.onResponse(null); + @Override + public void onAfter() { + if (outstanding.decrementAndGet() == 0) { + result.onResponse(null); + } } - } - }); - } + }); + } + + // Fetch next continuation token + continuationToken = page.getContinuationToken(); + } while (StringUtils.isNotBlank(continuationToken)); }); + if (outstanding.decrementAndGet() == 0) { result.onResponse(null); } @@ -301,20 +323,37 @@ public Map listBlobsByPrefix(String keyPath, String prefix .setPrefix(keyPath + (prefix == null ? "" : prefix)); SocketAccess.doPrivilegedVoidException(() -> { - for (final BlobItem blobItem : blobContainer.listBlobsByHierarchy("/", listBlobsOptions, timeout())) { - // Skipping over the prefixes, only look for the blobs - if (blobItem.isPrefix() != null && blobItem.isPrefix()) { - continue; + String continuationToken = null; + + do { + // Fetch one page at a time, others are going to be fetched by continuation token + final Optional> pageOpt = blobContainer.listBlobsByHierarchy("/", listBlobsOptions, timeout()) + .streamByPage(continuationToken) + .findFirst(); + + if (!pageOpt.isPresent()) { + // No more pages, should never happen + break; } - final String name = getBlobName(blobItem.getName(), container, keyPath); - logger.trace(() -> new ParameterizedMessage("blob name [{}]", name)); + final PagedResponse page = pageOpt.get(); + for (final BlobItem blobItem : page.getValue()) { + // Skipping over the prefixes, only look for the blobs + if (blobItem.isPrefix() != null && blobItem.isPrefix()) { + continue; + } - final BlobItemProperties properties = blobItem.getProperties(); - logger.trace(() -> new ParameterizedMessage("blob name [{}], size [{}]", name, properties.getContentLength())); - blobsBuilder.put(name, new PlainBlobMetadata(name, properties.getContentLength())); - } + final String name = getBlobName(blobItem.getName(), container, keyPath); + logger.trace(() -> new ParameterizedMessage("blob name [{}]", name)); + final BlobItemProperties properties = blobItem.getProperties(); + logger.trace(() -> new ParameterizedMessage("blob name [{}], size [{}]", name, properties.getContentLength())); + blobsBuilder.put(name, new PlainBlobMetadata(name, properties.getContentLength())); + } + + // Fetch next continuation token + continuationToken = page.getContinuationToken(); + } while (StringUtils.isNotBlank(continuationToken)); }); return MapBuilder.newMapBuilder(blobsBuilder).immutableMap(); @@ -330,18 +369,33 @@ public Map children(BlobPath path) throws URISyntaxExcept .setPrefix(keyPath); SocketAccess.doPrivilegedVoidException(() -> { - for (final BlobItem blobItem : blobContainer.listBlobsByHierarchy("/", listBlobsOptions, timeout())) { - // Skipping over the blobs, only look for prefixes - if (blobItem.isPrefix() != null && blobItem.isPrefix()) { - // Expecting name in the form /container/keyPath.* and we want to strip off the /container/ - // this requires 1 + container.length() + 1, with each 1 corresponding to one of the /. - // Lastly, we add the length of keyPath to the offset to strip this container's path. - final String name = getBlobName(blobItem.getName(), container, keyPath).replaceAll("/$", ""); - logger.trace(() -> new ParameterizedMessage("blob name [{}]", name)); - blobsBuilder.add(name); + String continuationToken = null; + + do { + final Optional> pageOpt = blobContainer.listBlobsByHierarchy("/", listBlobsOptions, timeout()) + .streamByPage(continuationToken) + .findFirst(); + + if (!pageOpt.isPresent()) { + // No more pages, should never happen + break; } - } - ; + + final PagedResponse page = pageOpt.get(); + for (final BlobItem blobItem : page.getValue()) { + // Skipping over the blobs, only look for prefixes + if (blobItem.isPrefix() != null && blobItem.isPrefix()) { + // Expecting name in the form /container/keyPath.* and we want to strip off the /container/ + // this requires 1 + container.length() + 1, with each 1 corresponding to one of the /. + // Lastly, we add the length of keyPath to the offset to strip this container's path. + final String name = getBlobName(blobItem.getName(), container, keyPath).replaceAll("/$", ""); + logger.trace(() -> new ParameterizedMessage("blob name [{}]", name)); + blobsBuilder.add(name); + } + } + // Fetch next continuation token + continuationToken = page.getContinuationToken(); + } while (StringUtils.isNotBlank(continuationToken)); }); return Collections.unmodifiableMap( From 16544b1e91070f15ce80ad2f9ab28c8281c3d520 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Thu, 16 Dec 2021 10:50:29 -0500 Subject: [PATCH 2/2] Added tests cases and TODO items, addressing code review comments Signed-off-by: Andriy Redko --- plugins/repository-azure/build.gradle | 21 ++++++++++++++++++- .../repositories/azure/AzureBlobStore.java | 7 +++++++ .../java/fixture/azure/AzureHttpHandler.java | 1 + 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/plugins/repository-azure/build.gradle b/plugins/repository-azure/build.gradle index 71356129845e5..e87d231a9e5ea 100644 --- a/plugins/repository-azure/build.gradle +++ b/plugins/repository-azure/build.gradle @@ -283,4 +283,23 @@ task azureThirdPartyTest(type: Test) { nonInputProperties.systemProperty 'test.azure.endpoint_suffix', "${-> azureAddress.call() }" } } -check.dependsOn(azureThirdPartyTest) + +task azureThirdPartyDefaultXmlTest(type: Test) { + SourceSetContainer sourceSets = project.getExtensions().getByType(SourceSetContainer.class); + SourceSet internalTestSourceSet = sourceSets.getByName(InternalClusterTestPlugin.SOURCE_SET_NAME) + setTestClassesDirs(internalTestSourceSet.getOutput().getClassesDirs()) + setClasspath(internalTestSourceSet.getRuntimeClasspath()) + dependsOn tasks.internalClusterTest + include '**/AzureStorageCleanupThirdPartyTests.class' + systemProperty 'javax.xml.stream.XMLInputFactory', "com.sun.xml.internal.stream.XMLInputFactoryImpl" + systemProperty 'test.azure.account', azureAccount ? azureAccount : "" + systemProperty 'test.azure.key', azureKey ? azureKey : "" + systemProperty 'test.azure.sas_token', azureSasToken ? azureSasToken : "" + systemProperty 'test.azure.container', azureContainer ? azureContainer : "" + systemProperty 'test.azure.base', (azureBasePath ? azureBasePath : "") + "_third_party_tests_" + BuildParams.testSeed + if (useFixture) { + nonInputProperties.systemProperty 'test.azure.endpoint_suffix', "${-> azureAddress.call() }" + } +} + +check.dependsOn(azureThirdPartyTest, azureThirdPartyDefaultXmlTest) diff --git a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java index 022b81b990e04..6345103c6ecc6 100644 --- a/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java +++ b/plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java @@ -224,6 +224,8 @@ public DeleteResult deleteBlobDirectory(String path, Executor executor) throws U do { // Fetch one page at a time, others are going to be fetched by continuation token + // TODO: reconsider reverting to simplified approach once https://github.com/Azure/azure-sdk-for-java/issues/26064 + // gets addressed. final Optional> pageOpt = blobContainer.listBlobs(listBlobsOptions, timeout()) .streamByPage(continuationToken) .findFirst(); @@ -327,6 +329,8 @@ public Map listBlobsByPrefix(String keyPath, String prefix do { // Fetch one page at a time, others are going to be fetched by continuation token + // TODO: reconsider reverting to simplified approach once https://github.com/Azure/azure-sdk-for-java/issues/26064 + // gets addressed final Optional> pageOpt = blobContainer.listBlobsByHierarchy("/", listBlobsOptions, timeout()) .streamByPage(continuationToken) .findFirst(); @@ -372,6 +376,9 @@ public Map children(BlobPath path) throws URISyntaxExcept String continuationToken = null; do { + // Fetch one page at a time, others are going to be fetched by continuation token + // TODO: reconsider reverting to simplified approach once https://github.com/Azure/azure-sdk-for-java/issues/26064 + // gets addressed final Optional> pageOpt = blobContainer.listBlobsByHierarchy("/", listBlobsOptions, timeout()) .streamByPage(continuationToken) .findFirst(); diff --git a/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java b/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java index 68ebeb5265fd0..f12a4579a2d0c 100644 --- a/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java +++ b/test/fixtures/azure-fixture/src/main/java/fixture/azure/AzureHttpHandler.java @@ -208,6 +208,7 @@ public void handle(final HttpExchange exchange) throws IOException { } list.append(""); + list.append(""); list.append(""); byte[] response = list.toString().getBytes(StandardCharsets.UTF_8);