From 967d2bf2e8780805b4acb56b1c852dde6de344b4 Mon Sep 17 00:00:00 2001 From: David Turner Date: Tue, 16 Apr 2019 12:19:33 +0100 Subject: [PATCH 1/2] Do not create missing directories in readonly repo Today we erroneously look for a node setting called `readonly` when deciding whether or not to create a missing directory in a filesystem repository. This change fixes this by using the repository setting instead. Closes #41009 Relates #26909 --- .../common/blobstore/fs/FsBlobStore.java | 13 ++-- .../repositories/fs/FsRepository.java | 2 +- .../fs/FsBlobStoreContainerTests.java | 2 +- .../common/blobstore/fs/FsBlobStoreTests.java | 8 +-- .../fs/FsBlobStoreRepositoryIT.java | 67 +++++++++++++++++++ .../snapshots/BlobStoreFormatIT.java | 3 +- 6 files changed, 82 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java b/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java index eea30dd4e530f..5074309b536b0 100644 --- a/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java +++ b/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java @@ -40,10 +40,10 @@ public class FsBlobStore implements BlobStore { private final boolean readOnly; - public FsBlobStore(Settings settings, Path path) throws IOException { + public FsBlobStore(Settings settings, Path path, boolean readonly) throws IOException { this.path = path; - this.readOnly = settings.getAsBoolean("readonly", false); - if (!this.readOnly) { + this.readOnly = readonly; + if (this.readOnly == false) { Files.createDirectories(path); } this.bufferSizeInBytes = (int) settings.getAsBytesSize("repositories.fs.buffer_size", @@ -74,6 +74,11 @@ public BlobContainer blobContainer(BlobPath path) { @Override public void delete(BlobPath path) throws IOException { + assert this.readOnly == false : "should not delete anything from a readonly repository: " + path; + //noinspection ConstantConditions in case assertions are disabled + if (this.readOnly) { + throw new ElasticsearchException("unexpectedly deleting [" + path + "] from a readonly repository"); + } IOUtils.rm(buildPath(path)); } @@ -84,7 +89,7 @@ public void close() { private synchronized Path buildAndCreate(BlobPath path) throws IOException { Path f = buildPath(path); - if (!readOnly) { + if (readOnly == false) { Files.createDirectories(f); } return f; diff --git a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java index 710e6aad40d16..8f495f2d4842a 100644 --- a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java @@ -108,7 +108,7 @@ public FsRepository(RepositoryMetaData metadata, Environment environment, NamedX protected BlobStore createBlobStore() throws Exception { final String location = REPOSITORIES_LOCATION_SETTING.get(metadata.settings()); final Path locationFile = environment.resolveRepoFile(location); - return new FsBlobStore(environment.settings(), locationFile); + return new FsBlobStore(environment.settings(), locationFile, isReadOnly()); } @Override diff --git a/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreContainerTests.java b/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreContainerTests.java index 9230cded82b1d..7bd24aec8de90 100644 --- a/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreContainerTests.java +++ b/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreContainerTests.java @@ -37,6 +37,6 @@ protected BlobStore newBlobStore() throws IOException { } else { settings = Settings.EMPTY; } - return new FsBlobStore(settings, createTempDir()); + return new FsBlobStore(settings, createTempDir(), false); } } diff --git a/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreTests.java b/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreTests.java index 59e4ffd7927ca..4a1b1e1016fb9 100644 --- a/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreTests.java +++ b/server/src/test/java/org/elasticsearch/common/blobstore/fs/FsBlobStoreTests.java @@ -42,15 +42,14 @@ protected BlobStore newBlobStore() throws IOException { } else { settings = Settings.EMPTY; } - return new FsBlobStore(settings, createTempDir()); + return new FsBlobStore(settings, createTempDir(), false); } public void testReadOnly() throws Exception { - Settings settings = Settings.builder().put("readonly", true).build(); Path tempDir = createTempDir(); Path path = tempDir.resolve("bar"); - try (FsBlobStore store = new FsBlobStore(settings, path)) { + try (FsBlobStore store = new FsBlobStore(Settings.EMPTY, path, true)) { assertFalse(Files.exists(path)); BlobPath blobPath = BlobPath.cleanPath().add("foo"); store.blobContainer(blobPath); @@ -61,8 +60,7 @@ public void testReadOnly() throws Exception { assertFalse(Files.exists(storePath)); } - settings = randomBoolean() ? Settings.EMPTY : Settings.builder().put("readonly", false).build(); - try (FsBlobStore store = new FsBlobStore(settings, path)) { + try (FsBlobStore store = new FsBlobStore(Settings.EMPTY, path, false)) { assertTrue(Files.exists(path)); BlobPath blobPath = BlobPath.cleanPath().add("foo"); BlobContainer container = store.blobContainer(blobPath); diff --git a/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java b/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java index 1ed42cb24746b..c84086f04ce09 100644 --- a/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java +++ b/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java @@ -18,12 +18,21 @@ */ package org.elasticsearch.repositories.fs; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.repositories.Repository; import org.elasticsearch.repositories.blobstore.ESBlobStoreRepositoryIntegTestCase; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.util.concurrent.ExecutionException; +import java.util.stream.Stream; + import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; import static org.hamcrest.Matchers.instanceOf; public class FsBlobStoreRepositoryIT extends ESBlobStoreRepositoryIntegTestCase { @@ -41,4 +50,62 @@ protected void createTestRepository(String name, boolean verify) { protected void afterCreationCheck(Repository repository) { assertThat(repository, instanceOf(FsRepository.class)); } + + public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOException, ExecutionException, InterruptedException { + final String repoName = randomAsciiName(); + logger.info("--> creating repository {}", repoName); + + final Path repoPath = randomRepoPath(); + + assertAcked(client().admin().cluster().preparePutRepository(repoName).setType("fs").setSettings(Settings.builder() + .put("location", repoPath) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES))); + + String indexName = randomAsciiName(); + int docCount = iterations(10, 1000); + logger.info("--> create random index {} with {} records", indexName, docCount); + addRandomDocuments(indexName, docCount); + assertHitCount(client().prepareSearch(indexName).setSize(0).get(), docCount); + + final String snapshotName = randomAsciiName(); + logger.info("--> create snapshot {}:{}", repoName, snapshotName); + assertSuccessfulSnapshot(client().admin().cluster().prepareCreateSnapshot(repoName, snapshotName) + .setWaitForCompletion(true).setIndices(indexName)); + + assertAcked(client().admin().indices().prepareDelete(indexName)); + assertAcked(client().admin().cluster().prepareDeleteRepository(repoName)); + + final Path deletedPath; + try (Stream contents = Files.list(repoPath.resolve("indices"))) { + //noinspection OptionalGetWithoutIsPresent because we know there's a subdirectory + deletedPath = contents.filter(Files::isDirectory).findAny().get(); + deleteRecursive(deletedPath); + } + assertFalse(Files.exists(deletedPath)); + + assertAcked(client().admin().cluster().preparePutRepository(repoName).setType("fs").setSettings(Settings.builder() + .put("location", repoPath).put("readonly", true))); + + final ElasticsearchException exception = expectThrows(ElasticsearchException.class, () -> + client().admin().cluster().prepareRestoreSnapshot(repoName, snapshotName).setWaitForCompletion(randomBoolean()).get()); + assertThat(exception.getRootCause(), instanceOf(NoSuchFileException.class)); + + assertFalse("deleted path is not recreated in readonly repository", Files.exists(deletedPath)); + } + + private void deleteRecursive(Path p) throws IOException { + if (Files.isDirectory(p)) { + try (Stream contents = Files.list(p)) { + contents.forEach(path -> { + try { + deleteRecursive(path); + } catch (IOException e) { + throw new AssertionError(e); + } + }); + } + } + Files.delete(p); + } } diff --git a/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java b/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java index 6f4f69ad67e88..4febd0695c936 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java +++ b/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java @@ -238,8 +238,7 @@ public void writeBlobAtomic(String blobName, InputStream inputStream, long blobS } protected BlobStore createTestBlobStore() throws IOException { - Settings settings = Settings.builder().build(); - return new FsBlobStore(settings, randomRepoPath()); + return new FsBlobStore(Settings.EMPTY, randomRepoPath(), false); } protected void randomCorruption(BlobContainer blobContainer, String blobName) throws IOException { From 0c322b0cc916ab8bedb2df64b4efc89a0919edd6 Mon Sep 17 00:00:00 2001 From: David Turner Date: Tue, 16 Apr 2019 13:56:17 +0100 Subject: [PATCH 2/2] Use IOUtils.rm --- .../common/blobstore/fs/FsBlobStore.java | 4 ++-- .../fs/FsBlobStoreRepositoryIT.java | 22 ++++--------------- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java b/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java index 5074309b536b0..8a4d51e4dc93c 100644 --- a/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java +++ b/server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobStore.java @@ -74,9 +74,9 @@ public BlobContainer blobContainer(BlobPath path) { @Override public void delete(BlobPath path) throws IOException { - assert this.readOnly == false : "should not delete anything from a readonly repository: " + path; + assert readOnly == false : "should not delete anything from a readonly repository: " + path; //noinspection ConstantConditions in case assertions are disabled - if (this.readOnly) { + if (readOnly) { throw new ElasticsearchException("unexpectedly deleting [" + path + "] from a readonly repository"); } IOUtils.rm(buildPath(path)); diff --git a/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java b/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java index c84086f04ce09..dd4ca7bfd20e4 100644 --- a/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java +++ b/server/src/test/java/org/elasticsearch/repositories/fs/FsBlobStoreRepositoryIT.java @@ -21,6 +21,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeUnit; +import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.repositories.Repository; import org.elasticsearch.repositories.blobstore.ESBlobStoreRepositoryIntegTestCase; @@ -53,10 +54,10 @@ protected void afterCreationCheck(Repository repository) { public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOException, ExecutionException, InterruptedException { final String repoName = randomAsciiName(); - logger.info("--> creating repository {}", repoName); - final Path repoPath = randomRepoPath(); + logger.info("--> creating repository {} at {}", repoName, repoPath); + assertAcked(client().admin().cluster().preparePutRepository(repoName).setType("fs").setSettings(Settings.builder() .put("location", repoPath) .put("compress", randomBoolean()) @@ -80,7 +81,7 @@ public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOExce try (Stream contents = Files.list(repoPath.resolve("indices"))) { //noinspection OptionalGetWithoutIsPresent because we know there's a subdirectory deletedPath = contents.filter(Files::isDirectory).findAny().get(); - deleteRecursive(deletedPath); + IOUtils.rm(deletedPath); } assertFalse(Files.exists(deletedPath)); @@ -93,19 +94,4 @@ public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOExce assertFalse("deleted path is not recreated in readonly repository", Files.exists(deletedPath)); } - - private void deleteRecursive(Path p) throws IOException { - if (Files.isDirectory(p)) { - try (Stream contents = Files.list(p)) { - contents.forEach(path -> { - try { - deleteRecursive(path); - } catch (IOException e) { - throw new AssertionError(e); - } - }); - } - } - Files.delete(p); - } }