From d0c2b5bc1be408a90a2f42df9d1107996dd74c59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dharmesh=20=F0=9F=92=A4?= Date: Wed, 30 Aug 2023 14:33:36 +0530 Subject: [PATCH] Removing verifyLocally and move to existing verify methods. If the repository is a system repository only local verification will happen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Dharmesh 💤 --- .../cluster/remotestore/RemoteStoreNode.java | 4 + .../remotestore/RemoteStoreService.java | 4 +- .../repositories/FilterRepository.java | 8 +- .../opensearch/repositories/Repository.java | 14 +- .../blobstore/BlobStoreRepository.java | 133 ++++++------------ .../RepositoriesServiceTests.java | 6 +- .../index/shard/RestoreOnlyRepository.java | 8 +- 7 files changed, 70 insertions(+), 107 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 1b49ccb06f917..a6bd099679aa4 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 @@ -12,6 +12,7 @@ import org.opensearch.cluster.metadata.RepositoryMetadata; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.common.settings.Settings; +import org.opensearch.repositories.blobstore.BlobStoreRepository; import java.util.ArrayList; import java.util.HashSet; @@ -82,6 +83,9 @@ private RepositoryMetadata buildRepositoryMetadata(String name) { Settings.Builder settings = Settings.builder(); settingsMap.forEach(settings::put); + // Repository metadata built here will always be for a system repository. + settings.put(BlobStoreRepository.SYSTEM_REPOSITORY_SETTING.getKey(), true); + return new RepositoryMetadata(name, type, settings.build()); } 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 9309771ed6fb9..e2cee2f2f6449 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 @@ -87,7 +87,9 @@ public RemoteStoreService(Supplier repositoriesService, Thr public void verifyRepositoriesLocally(List repositories, DiscoveryNode localNode) { for (Repository repository : repositories) { String repositoryName = repository.getMetadata().name(); - repository.verifyLocally(localNode); + String verificationToken = repository.startVerification(); + repository.verify(verificationToken, localNode); + repository.endVerification(verificationToken); logger.info(() -> new ParameterizedMessage("successfully verified [{}] repository", repositoryName)); } } diff --git a/server/src/main/java/org/opensearch/repositories/FilterRepository.java b/server/src/main/java/org/opensearch/repositories/FilterRepository.java index 21904d9799476..697ac37c4a175 100644 --- a/server/src/main/java/org/opensearch/repositories/FilterRepository.java +++ b/server/src/main/java/org/opensearch/repositories/FilterRepository.java @@ -163,13 +163,13 @@ public void verify(String verificationToken, DiscoveryNode localNode) { } @Override - public void verifyLocally(DiscoveryNode localNode) { - in.verifyLocally(localNode); + public boolean isReadOnly() { + return in.isReadOnly(); } @Override - public boolean isReadOnly() { - return in.isReadOnly(); + public boolean isSystemRepository() { + return in.isSystemRepository(); } @Override diff --git a/server/src/main/java/org/opensearch/repositories/Repository.java b/server/src/main/java/org/opensearch/repositories/Repository.java index 1e2c1e0398ba7..10f3dc2b6b340 100644 --- a/server/src/main/java/org/opensearch/repositories/Repository.java +++ b/server/src/main/java/org/opensearch/repositories/Repository.java @@ -241,19 +241,19 @@ 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 */ boolean isReadOnly(); + /** + * Returns true if the repository is managed by the system directly and doesn't allow managing the lifetime of the + * repository through external APIs + * @return true if the repository is system managed + */ + boolean isSystemRepository(); + /** * Creates a snapshot of the shard based on the index commit point. *

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 2ecc6aeedaf37..35e875631feb0 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -285,6 +285,15 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp */ public static final Setting READONLY_SETTING = Setting.boolSetting("readonly", false, Setting.Property.NodeScope); + /*** + * Setting to set repository as system repository + */ + public static final Setting SYSTEM_REPOSITORY_SETTING = Setting.boolSetting( + "system_repository", + false, + Setting.Property.NodeScope + ); + protected final boolean supportURLRepo; private final int maxShardBlobDeleteBatch; @@ -346,6 +355,8 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp private final boolean readOnly; + private final boolean isSystemRepository; + private final Object lock = new Object(); private final SetOnce blobContainer = new SetOnce<>(); @@ -411,6 +422,7 @@ protected BlobStoreRepository( remoteUploadRateLimiter = getRateLimiter(metadata.settings(), "max_remote_upload_bytes_per_sec", ByteSizeValue.ZERO); remoteDownloadRateLimiter = getRateLimiter(metadata.settings(), "max_remote_download_bytes_per_sec", ByteSizeValue.ZERO); readOnly = READONLY_SETTING.get(metadata.settings()); + isSystemRepository = SYSTEM_REPOSITORY_SETTING.get(metadata.settings()); cacheRepositoryData = CACHE_REPOSITORY_DATA.get(metadata.settings()); bufferSize = Math.toIntExact(BUFFER_SIZE_SETTING.get(metadata.settings()).getBytes()); maxShardBlobDeleteBatch = MAX_SNAPSHOT_SHARD_BLOB_DELETE_BATCH_SIZE.get(metadata.settings()); @@ -1823,8 +1835,10 @@ public String startVerification() { byte[] testBytes = Strings.toUTF8Bytes(seed); BlobContainer testContainer = blobStore().blobContainer(basePath().add(testBlobPrefix(seed))); BytesArray bytes = new BytesArray(testBytes); - try (InputStream stream = bytes.streamInput()) { - testContainer.writeBlobAtomic("master.dat", stream, bytes.length(), true); + if (isSystemRepository == false) { + try (InputStream stream = bytes.streamInput()) { + testContainer.writeBlobAtomic("master.dat", stream, bytes.length(), true); + } } return seed; } @@ -2133,6 +2147,11 @@ public boolean isReadOnly() { return readOnly; } + @Override + public boolean isSystemRepository() { + return isSystemRepository; + } + /** * Writing a new index generation is a three step process. * First, the {@link RepositoryMetadata} entry for this repository is set into a pending state by incrementing its @@ -3148,99 +3167,33 @@ public void verify(String seed, DiscoveryNode localNode) { exp ); } - try (InputStream masterDat = testBlobContainer.readBlob("master.dat")) { - final String seedRead = Streams.readFully(masterDat).utf8ToString(); - if (seedRead.equals(seed) == false) { - throw new RepositoryVerificationException( - metadata.name(), - "Seed read from master.dat was [" + seedRead + "] but expected seed [" + seed + "]" - ); - } - } catch (NoSuchFileException e) { - throw new RepositoryVerificationException( - metadata.name(), - "a file written by cluster-manager to the store [" - + blobStore() - + "] cannot be accessed on the node [" - + localNode - + "]. " - + "This might indicate that the store [" - + blobStore() - + "] is not shared between this node and the cluster-manager node or " - + "that permissions on the store don't allow reading files written by the cluster-manager node", - e - ); - } catch (Exception e) { - throw new RepositoryVerificationException(metadata.name(), "Failed to verify repository", e); - } - } - } - - @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) { + if (isSystemRepository == false) { + try (InputStream masterDat = testBlobContainer.readBlob("master.dat")) { + final String seedRead = Streams.readFully(masterDat).utf8ToString(); + if (seedRead.equals(seed) == false) { + throw new RepositoryVerificationException( + metadata.name(), + "Seed read from master.dat was [" + seedRead + "] but expected seed [" + seed + "]" + ); + } + } catch (NoSuchFileException e) { throw new RepositoryVerificationException( metadata.name(), - "Seed read was [" + seedRead + "] but expected seed [" + seed + "]" + "a file written by cluster-manager to the store [" + + blobStore() + + "] cannot be accessed on the node [" + + localNode + + "]. " + + "This might indicate that the store [" + + blobStore() + + "] is not shared between this node and the cluster-manager node or " + + "that permissions on the store don't allow reading files written by the cluster-manager node", + e ); + } catch (Exception e) { + throw new RepositoryVerificationException(metadata.name(), "Failed to verify repository", e); } - } 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)); } } } diff --git a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java index b71bc0778ad47..e5aa194949922 100644 --- a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java @@ -307,10 +307,12 @@ public void verify(String verificationToken, DiscoveryNode localNode) { } @Override - public void verifyLocally(DiscoveryNode localNode) {} + public boolean isReadOnly() { + return false; + } @Override - public boolean isReadOnly() { + public boolean isSystemRepository() { return false; } 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 f012e516f7904..be2f895301396 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 @@ -173,6 +173,11 @@ public boolean isReadOnly() { return false; } + @Override + public boolean isSystemRepository() { + return false; + } + @Override public void snapshotShard( Store store, @@ -195,9 +200,6 @@ 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) {}