From bfa06d963e6161c337d2b40b41295f42379aeeeb Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 17 Apr 2019 08:34:07 +0100 Subject: [PATCH] Do not create missing directories in readonly repo (#41249) 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 | 53 +++++++++++++++++++ .../snapshots/BlobStoreFormatIT.java | 3 +- 6 files changed, 68 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 eea30dd4e53..8a4d51e4dc9 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 class FsBlobStore implements BlobStore { @Override public void delete(BlobPath path) throws IOException { + assert readOnly == false : "should not delete anything from a readonly repository: " + path; + //noinspection ConstantConditions in case assertions are disabled + if (readOnly) { + throw new ElasticsearchException("unexpectedly deleting [" + path + "] from a readonly repository"); + } IOUtils.rm(buildPath(path)); } @@ -84,7 +89,7 @@ public class FsBlobStore implements BlobStore { 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 ea438f03bf1..a47ced0496d 100644 --- a/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java @@ -115,7 +115,7 @@ public class FsRepository extends BlobStoreRepository { 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 9230cded82b..7bd24aec8de 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 @@ public class FsBlobStoreContainerTests extends ESBlobStoreContainerTestCase { } 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 59e4ffd7927..4a1b1e1016f 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 @@ public class FsBlobStoreTests extends ESBlobStoreTestCase { } 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 class FsBlobStoreTests extends ESBlobStoreTestCase { 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 1ed42cb2474..dd4ca7bfd20 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,22 @@ */ package org.elasticsearch.repositories.fs; +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; +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 +51,47 @@ public class FsBlobStoreRepositoryIT extends ESBlobStoreRepositoryIntegTestCase protected void afterCreationCheck(Repository repository) { assertThat(repository, instanceOf(FsRepository.class)); } + + public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOException, ExecutionException, InterruptedException { + final String repoName = randomAsciiName(); + 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()) + .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(); + IOUtils.rm(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)); + } } diff --git a/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java b/server/src/test/java/org/elasticsearch/snapshots/BlobStoreFormatIT.java index 6f4f69ad67e..4febd0695c9 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 class BlobStoreFormatIT extends AbstractSnapshotIntegTestCase { } 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 {