From bc0e5621a26e5146edac734e48666729834e605d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dharmesh=20=F0=9F=92=A4?= Date: Mon, 28 Aug 2023 19:42:26 +0530 Subject: [PATCH] Introducing verifylocally to perform verification without cluster being formed such that a node is able to connect to connect and has necessary permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Dharmesh 💤 --- .../cluster/remotestore/RemoteStoreNode.java | 24 +++--- .../remotestore/RemoteStoreService.java | 48 +----------- .../cluster/node/DiscoveryNode.java | 2 +- .../repositories/FilterRepository.java | 5 ++ .../opensearch/repositories/Repository.java | 7 ++ .../blobstore/BlobStoreRepository.java | 75 +++++++++++++++++-- .../index/shard/RestoreOnlyRepository.java | 3 + 7 files changed, 101 insertions(+), 63 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreNode.java b/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreNode.java index 1176a7fcd2c81..1b49ccb06f917 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreNode.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreNode.java @@ -14,11 +14,12 @@ import org.opensearch.common.settings.Settings; import java.util.ArrayList; -import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.stream.Collectors; /** @@ -85,19 +86,20 @@ private RepositoryMetadata buildRepositoryMetadata(String name) { } private RepositoriesMetadata buildRepositoriesMetadata() { - String segmentRepositoryName = validateAttributeNonNull(REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY); - String translogRepositoryName = validateAttributeNonNull(REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY); - if (segmentRepositoryName.equals(translogRepositoryName)) { - return new RepositoriesMetadata(Collections.singletonList(buildRepositoryMetadata(segmentRepositoryName))); - } else { - List repositoryMetadataList = new ArrayList<>(); - repositoryMetadataList.add(buildRepositoryMetadata(segmentRepositoryName)); - repositoryMetadataList.add(buildRepositoryMetadata(translogRepositoryName)); - return new RepositoriesMetadata(repositoryMetadataList); + List repositoryMetadataList = new ArrayList<>(); + Set repositoryNames = new HashSet<>(); + + repositoryNames.add(validateAttributeNonNull(REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY)); + repositoryNames.add(validateAttributeNonNull(REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY)); + + for (String repositoryName : repositoryNames) { + repositoryMetadataList.add(buildRepositoryMetadata(repositoryName)); } + + return new RepositoriesMetadata(repositoryMetadataList); } - RepositoriesMetadata getRepositoriesMetadata() { + public RepositoriesMetadata getRepositoriesMetadata() { return this.repositoriesMetadata; } diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java b/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java index d69f3ffc39188..0fef8389063a1 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/remotestore/RemoteStoreService.java @@ -19,15 +19,12 @@ import org.opensearch.common.settings.Setting; import org.opensearch.repositories.RepositoriesService; import org.opensearch.repositories.Repository; -import org.opensearch.repositories.RepositoryVerificationException; import org.opensearch.threadpool.ThreadPool; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Locale; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; import java.util.function.Supplier; /** @@ -87,50 +84,11 @@ public RemoteStoreService(Supplier repositoriesService, Thr * repository mentioned. This verification will happen on a local node to validate if the node is able to connect * to the repository. */ - public void verifyRepository(List repositories, DiscoveryNode localNode) { - /* + public void verifyRepositoriesLocally(List repositories, DiscoveryNode localNode) { for (Repository repository : repositories) { - String verificationToken = repository.startVerification(); String repositoryName = repository.getMetadata().name(); - try { - repository.verify(verificationToken, localNode); - logger.info(() -> new ParameterizedMessage("successfully verified [{}] repository", repositoryName)); - } catch (Exception e) { - logger.warn(() -> new ParameterizedMessage("[{}] failed to verify repository", repository), e); - throw new RepositoryVerificationException(repositoryName, e.getMessage()); - } - } - Replace the below code with this once #9088 is merged. - */ - - for (Repository repository : repositories) { - String verificationToken = repository.startVerification(); - String repositoryName = repository.getMetadata().name(); - CountDownLatch repositoryVerificationLatch = new CountDownLatch(1); - threadPool.executor(ThreadPool.Names.GENERIC).execute(() -> { - try { - repository.verify(verificationToken, localNode); - logger.info(() -> new ParameterizedMessage("successfully verified [{}] repository", repositoryName)); - repositoryVerificationLatch.countDown(); - } catch (Exception e) { - logger.warn(() -> new ParameterizedMessage("[{}] failed to verify repository", repository), e); - throw new RepositoryVerificationException(repositoryName, e.getMessage()); - } - }); - - // TODO: See if using listener here which is async makes sense, made this sync as - // we need the repository registration for remote store backed node to be completed before the - // bootstrap completes. - try { - if (repositoryVerificationLatch.await(1000, TimeUnit.MILLISECONDS) == false) { - throw new RepositoryVerificationException( - repository.getMetadata().name(), - "could not complete " + "repository verification within timeout." - ); - } - } catch (InterruptedException e) { - throw new RepositoryVerificationException(repository.getMetadata().name(), e.getMessage()); - } + repository.verifyLocally(localNode); + logger.info(() -> new ParameterizedMessage("successfully verified [{}] repository", repositoryName)); } } diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java index 0fb8a54209a1b..30f5be2730ac9 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java @@ -305,7 +305,7 @@ public static DiscoveryNode createLocal( ); RemoteStoreNode remoteStoreNode = new RemoteStoreNode(discoveryNode); List repositories = remoteStoreService.createRepositories(remoteStoreNode); - remoteStoreService.verifyRepository(repositories, discoveryNode); + remoteStoreService.verifyRepositoriesLocally(repositories, discoveryNode); return discoveryNode; } diff --git a/server/src/main/java/org/opensearch/repositories/FilterRepository.java b/server/src/main/java/org/opensearch/repositories/FilterRepository.java index 08f8bcb467d03..21904d9799476 100644 --- a/server/src/main/java/org/opensearch/repositories/FilterRepository.java +++ b/server/src/main/java/org/opensearch/repositories/FilterRepository.java @@ -162,6 +162,11 @@ public void verify(String verificationToken, DiscoveryNode localNode) { in.verify(verificationToken, localNode); } + @Override + public void verifyLocally(DiscoveryNode localNode) { + in.verifyLocally(localNode); + } + @Override public boolean isReadOnly() { return in.isReadOnly(); diff --git a/server/src/main/java/org/opensearch/repositories/Repository.java b/server/src/main/java/org/opensearch/repositories/Repository.java index 76a3b65c9ea55..1e2c1e0398ba7 100644 --- a/server/src/main/java/org/opensearch/repositories/Repository.java +++ b/server/src/main/java/org/opensearch/repositories/Repository.java @@ -241,6 +241,13 @@ default RepositoryStats stats() { */ void verify(String verificationToken, DiscoveryNode localNode); + /** + * Verifies repository settings on local node by reading and writing files onto blobstore without the + * cluster-manager. + * @param localNode the local node information + */ + void verifyLocally(DiscoveryNode localNode); + /** * Returns true if the repository supports only read operations * @return true if the repository is read/only diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java index d84381cfc790e..2ecc6aeedaf37 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -3123,12 +3123,6 @@ public IndexShardSnapshotStatus getShardSnapshotStatus(SnapshotId snapshotId, In @Override public void verify(String seed, DiscoveryNode localNode) { - /* - if(!isSystemRepository) { - assertSnapshotOrGenericThread(); - } - Update the assertion method invocation with this once #9088 is merged. - */ assertSnapshotOrGenericThread(); if (isReadOnly()) { try { @@ -3182,6 +3176,75 @@ public void verify(String seed, DiscoveryNode localNode) { } } + @Override + public void verifyLocally(DiscoveryNode localNode) { + String seed = UUIDs.randomBase64UUID(); + if (isReadOnly()) { + try { + latestIndexBlobId(); + } catch (Exception e) { + throw new RepositoryVerificationException( + metadata.name(), + "path " + basePath() + " is not accessible on node " + localNode, + e + ); + } + } else { + BlobContainer testBlobContainer = blobStore().blobContainer(basePath().add(testBlobPrefix(seed))); + String blobName = "data-" + localNode.getId() + ".dat"; + + // Writing test data to the repository + try { + BytesArray bytes = new BytesArray(seed); + try (InputStream stream = bytes.streamInput()) { + testBlobContainer.writeBlob(blobName, stream, bytes.length(), true); + } + } catch (Exception exp) { + throw new RepositoryVerificationException( + metadata.name(), + "store location [" + blobStore() + "] is not accessible on the node [" + localNode + "]", + exp + ); + } + + // Reading test data from the repository + try (InputStream localNodeDat = testBlobContainer.readBlob(blobName)) { + final String seedRead = Streams.readFully(localNodeDat).utf8ToString(); + if (seedRead.equals(seed) == false) { + throw new RepositoryVerificationException( + metadata.name(), + "Seed read was [" + seedRead + "] but expected seed [" + seed + "]" + ); + } + } catch (NoSuchFileException e) { + throw new RepositoryVerificationException( + metadata.name(), + "a file written to the store [" + + blobStore() + + "] cannot be accessed on the node [" + + localNode + + "]. " + + "This might indicate that the store [" + + blobStore() + + "] permissions don't allow reading files", + e + ); + } catch (Exception e) { + throw new RepositoryVerificationException(metadata.name(), "Failed to verify repository", e); + } + + // Trying to delete the repository once the write and read verification completes. We wont fail the + // verification if the detete fails. + // TODO: See if there is a better way to handle this deletion failure. + try { + final String testPrefix = testBlobPrefix(seed); + blobStore().blobContainer(basePath().add(testPrefix)).delete(); + } catch (Exception exp) { + logger.warn(() -> new ParameterizedMessage("[{}] cannot delete test data at {} {}", metadata.name(), basePath(), exp)); + } + } + } + @Override public String toString() { return "BlobStoreRepository[" + "[" + metadata.name() + "], [" + blobStore.get() + ']' + ']'; diff --git a/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java b/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java index fbee13ab3b551..f012e516f7904 100644 --- a/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java +++ b/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java @@ -195,6 +195,9 @@ public IndexShardSnapshotStatus getShardSnapshotStatus(SnapshotId snapshotId, In @Override public void verify(String verificationToken, DiscoveryNode localNode) {} + @Override + public void verifyLocally(DiscoveryNode localNode) {} + @Override public void updateState(final ClusterState state) {}