Skip to content

Commit

Permalink
Fix remove of azure files
Browse files Browse the repository at this point in the history 
Probably when we updated Azure SDK, we introduced a regression.
Actually, we are not able to remove files anymore.

For example, if you register a new azure repository, the snapshot service tries to create a temp file and then remove it.
Removing does not work and you can see in logs:

```
[2016-05-18 11:03:24,914][WARN ][org.elasticsearch.cloud.azure.blobstore] [azure] can not remove [tests-ilmRPJ8URU-sh18yj38O6g/] in container {elasticsearch-snapshots}: The specified blob does not exist.
```

This fix deals with that. It now list all the files in a flatten mode, remove in the full URL the server and the container name.

As an example, when you are removing a blob which full name is `https://dpi24329.blob.core.windows.net/elasticsearch-snapshots/bar/test` you need to actually call Azure SDK with `bar/test` as the path, `elasticsearch-snapshots` is the container.

Related to #16472.
Related to #18436.

Backport of #18451 in 2.x branch

To test it, I ran some manual tests:

On my laptop, create a file `/path/to/azure/config/elasticsearch.yml`:

```yml
cloud.azure.storage.default.account: ACCOUNT
cloud.azure.storage.default.key: KEY
```

Run `AzureRepositoryF#main()` with `-Des.cluster.routing.allocation.disk.threshold_enabled=false -Des.path.home=/path/to/azure/` options.

Then run:

```sh
curl -XDELETE localhost:9200/foo?pretty
curl -XDELETE localhost:9200/_snapshot/my_backup1?pretty
curl -XPUT localhost:9200/foo/bar/1?pretty -d '{
 "foo": "bar"
}'
curl -XPOST localhost:9200/foo/_refresh?pretty
curl -XGET localhost:9200/foo/_count?pretty
curl -XPUT localhost:9200/_snapshot/my_backup1?pretty -d '{
   "type": "azure"
}'

curl -XPOST "localhost:9200/_snapshot/my_backup1/snap1?pretty&wait_for_completion=true"
curl -XDELETE localhost:9200/foo?pretty
curl -XPOST "localhost:9200/_snapshot/my_backup1/snap1/_restore?pretty&wait_for_completion=true"
curl -XGET localhost:9200/foo/_count?pretty
```

Then check files we have on azure platform using the console.
Then run:

```
curl -XDELETE localhost:9200/_snapshot/my_backup1/snap1?pretty
```

Then check files we have on azure platform using the console and verify that everything has been cleaned.
  • Loading branch information
@dadoonet
dadoonet committed May 25, 2016
1 parent beb759c commit 92382d8
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 21 deletions.
7 changes: 7 additions & 0 deletions ...cloud-azure/src/main/java/org/elasticsearch/cloud/azure/blobstore/AzureBlobContainer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public AzureBlobContainer(String repositoryName, BlobPath path, AzureBlobStore b

@Override
public boolean blobExists(String blobName) {
logger.trace("blobExists({})", blobName);
try {
return blobStore.blobExists(blobStore.container(), buildKey(blobName));
} catch (URISyntaxException | StorageException e) {
Expand All @@ -70,6 +71,7 @@ public boolean blobExists(String blobName) {

@Override
public InputStream openInput(String blobName) throws IOException {
logger.trace("openInput({})", blobName);
try {
return blobStore.getInputStream(blobStore.container(), buildKey(blobName));
} catch (StorageException e) {
Expand All @@ -84,6 +86,7 @@ public InputStream openInput(String blobName) throws IOException {

@Override
public OutputStream createOutput(String blobName) throws IOException {
logger.trace("createOutput({})", blobName);
try {
return new AzureOutputStream(blobStore.getOutputStream(blobStore.container(), buildKey(blobName)));
} catch (StorageException e) {
Expand All @@ -100,6 +103,7 @@ public OutputStream createOutput(String blobName) throws IOException {

@Override
public void deleteBlob(String blobName) throws IOException {
logger.trace("deleteBlob({})", blobName);
try {
blobStore.deleteBlob(blobStore.container(), buildKey(blobName));
} catch (URISyntaxException | StorageException e) {
Expand All @@ -110,6 +114,7 @@ public void deleteBlob(String blobName) throws IOException {

@Override
public Map<String, BlobMetaData> listBlobsByPrefix(@Nullable String prefix) throws IOException {
logger.trace("listBlobsByPrefix({})", prefix);

try {
return blobStore.listBlobsByPrefix(blobStore.container(), keyPath, prefix);
Expand All @@ -121,6 +126,7 @@ public Map<String, BlobMetaData> listBlobsByPrefix(@Nullable String prefix) thro

@Override
public void move(String sourceBlobName, String targetBlobName) throws IOException {
logger.trace("move({}, {})", sourceBlobName, targetBlobName);
try {
String source = keyPath + sourceBlobName;
String target = keyPath + targetBlobName;
Expand All @@ -139,6 +145,7 @@ public void move(String sourceBlobName, String targetBlobName) throws IOExceptio

@Override
public Map<String, BlobMetaData> listBlobs() throws IOException {
logger.trace("listBlobs()");
return listBlobsByPrefix(null);
}

Expand Down
64 changes: 43 additions & 21 deletions ...ud-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ CloudBlobClient getSelectedClient(String account, LocationMode mode) {
public boolean doesContainerExist(String account, LocationMode mode, String container) {
try {
CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blob_container = client.getContainerReference(container);
return blob_container.exists();
CloudBlobContainer blobContainer = client.getContainerReference(container);
return blobContainer.exists();
} catch (Exception e) {
logger.error("can not access container [{}]", container);
}
Expand All @@ -144,25 +144,25 @@ public boolean doesContainerExist(String account, LocationMode mode, String cont
@Override
public void removeContainer(String account, LocationMode mode, String container) throws URISyntaxException, StorageException {
CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blob_container = client.getContainerReference(container);
CloudBlobContainer blobContainer = client.getContainerReference(container);
// TODO Should we set some timeout and retry options?
/*
BlobRequestOptions options = new BlobRequestOptions();
options.setTimeoutIntervalInMs(1000);
options.setRetryPolicyFactory(new RetryNoRetry());
blob_container.deleteIfExists(options, null);
blobContainer.deleteIfExists(options, null);
*/
logger.trace("removing container [{}]", container);
blob_container.deleteIfExists();
blobContainer.deleteIfExists();
}

@Override
public void createContainer(String account, LocationMode mode, String container) throws URISyntaxException, StorageException {
try {
CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blob_container = client.getContainerReference(container);
CloudBlobContainer blobContainer = client.getContainerReference(container);
logger.trace("creating container [{}]", container);
blob_container.createIfNotExists();
blobContainer.createIfNotExists();
} catch (IllegalArgumentException e) {
logger.trace("fails creating container [{}]", container, e.getMessage());
throw new RepositoryException(container, e.getMessage());
Expand All @@ -175,22 +175,44 @@ public void deleteFiles(String account, LocationMode mode, String container, Str

// Container name must be lower case.
CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blob_container = client.getContainerReference(container);
if (blob_container.exists()) {
for (ListBlobItem blobItem : blob_container.listBlobs(path)) {
logger.trace("removing blob [{}]", blobItem.getUri());
deleteBlob(account, mode, container, blobItem.getUri().toString());
CloudBlobContainer blobContainer = client.getContainerReference(container);
if (blobContainer.exists()) {
// We list the blobs using a flat blob listing mode
for (ListBlobItem blobItem : blobContainer.listBlobs(path, true)) {
String blobName = blobNameFromUri(blobItem.getUri());
logger.trace("removing blob [{}] full URI was [{}]", blobName, blobItem.getUri());
deleteBlob(account, mode, container, blobName);
}
}
}

/**
* Extract the blob name from a URI like https://myservice.azure.net/container/path/to/myfile
* It should remove the container part (first part of the path) and gives path/to/myfile
* @param uri URI to parse
* @return The blob name relative to the container
*/
public static String blobNameFromUri(URI uri) {
String path = uri.getPath();

// We remove the container name from the path
// The 3 magic number cames from the fact if path is /container/path/to/myfile
// First occurrence is empty "/"
// Second occurrence is "container
// Last part contains "path/to/myfile" which is what we want to get
String[] splits = path.split("/", 3);

// We return the remaining end of the string
return splits[2];
}

@Override
public boolean blobExists(String account, LocationMode mode, String container, String blob) throws URISyntaxException, StorageException {
// Container name must be lower case.
CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blob_container = client.getContainerReference(container);
if (blob_container.exists()) {
CloudBlockBlob azureBlob = blob_container.getBlockBlobReference(blob);
CloudBlobContainer blobContainer = client.getContainerReference(container);
if (blobContainer.exists()) {
CloudBlockBlob azureBlob = blobContainer.getBlockBlobReference(blob);
return azureBlob.exists();
}

Expand All @@ -203,10 +225,10 @@ public void deleteBlob(String account, LocationMode mode, String container, Stri

// Container name must be lower case.
CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blob_container = client.getContainerReference(container);
if (blob_container.exists()) {
CloudBlobContainer blobContainer = client.getContainerReference(container);
if (blobContainer.exists()) {
logger.trace("container [{}]: blob [{}] found. removing.", container, blob);
CloudBlockBlob azureBlob = blob_container.getBlockBlobReference(blob);
CloudBlockBlob azureBlob = blobContainer.getBlockBlobReference(blob);
azureBlob.delete();
}
}
Expand Down Expand Up @@ -266,10 +288,10 @@ public void moveBlob(String account, LocationMode mode, String container, String
logger.debug("moveBlob container [{}], sourceBlob [{}], targetBlob [{}]", container, sourceBlob, targetBlob);

CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blob_container = client.getContainerReference(container);
CloudBlockBlob blobSource = blob_container.getBlockBlobReference(sourceBlob);
CloudBlobContainer blobContainer = client.getContainerReference(container);
CloudBlockBlob blobSource = blobContainer.getBlockBlobReference(sourceBlob);
if (blobSource.exists()) {
CloudBlockBlob blobTarget = blob_container.getBlockBlobReference(targetBlob);
CloudBlockBlob blobTarget = blobContainer.getBlockBlobReference(targetBlob);
blobTarget.startCopy(blobSource);
blobSource.delete();
logger.debug("moveBlob container [{}], sourceBlob [{}], targetBlob [{}] -> done", container, sourceBlob, targetBlob);
Expand Down
13 changes: 13 additions & 0 deletions ...ud-azure/src/test/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
import org.elasticsearch.test.ESTestCase;

import java.net.URI;
import java.net.URISyntaxException;

import static org.elasticsearch.cloud.azure.storage.AzureStorageServiceImpl.blobNameFromUri;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;

Expand Down Expand Up @@ -168,4 +170,15 @@ void createClient(AzureStorageSettings azureStorageSettings) {
new CloudBlobClient(URI.create("https://" + azureStorageSettings.getName())));
}
}

public void testBlobNameFromUri() throws URISyntaxException {
String name = blobNameFromUri(new URI("https://myservice.azure.net/container/path/to/myfile"));
assertThat(name, is("path/to/myfile"));
name = blobNameFromUri(new URI("http://myservice.azure.net/container/path/to/myfile"));
assertThat(name, is("path/to/myfile"));
name = blobNameFromUri(new URI("http://127.0.0.1/container/path/to/myfile"));
assertThat(name, is("path/to/myfile"));
name = blobNameFromUri(new URI("https://127.0.0.1/container/path/to/myfile"));
assertThat(name, is("path/to/myfile"));
}
}

0 comments on commit 92382d8

Please sign in to comment.