From e21d93710b5cb73ec30a4f62130c215a447f2601 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Fri, 23 Aug 2024 23:29:24 +0530 Subject: [PATCH 01/12] Add snapshot shard blobs with hashed prefix Signed-off-by: Ashish Singh --- CHANGELOG.md | 1 + .../recovery/RemoteStoreRestoreService.java | 3 +- .../index/remote/RemoteIndexPathUploader.java | 3 +- .../index/remote/RemoteStoreEnums.java | 4 +- .../index/remote/RemoteStorePathStrategy.java | 72 ++- .../org/opensearch/repositories/IndexId.java | 38 +- .../repositories/RepositoryData.java | 30 +- .../blobstore/BaseBlobStoreFormat.java | 58 +- .../blobstore/BlobStoreRepository.java | 603 ++++++++++++++---- .../blobstore/ChecksumBlobStoreFormat.java | 40 +- .../blobstore/ConfigBlobStoreFormat.java | 26 +- .../snapshots/SnapshotShardPaths.java | 95 +++ .../snapshots/SnapshotsService.java | 7 +- .../repositories/RepositoryDataTests.java | 2 +- .../snapshots/SnapshotShardPathsTests.java | 103 +++ 15 files changed, 902 insertions(+), 183 deletions(-) create mode 100644 server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java create mode 100644 server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index a9469846b1648..18b30361dc57d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add allowlist setting for ingest-geoip and ingest-useragent ([#15325](https://github.com/opensearch-project/OpenSearch/pull/15325)) - Adding access to noSubMatches and noOverlappingMatches in Hyphenation ([#13895](https://github.com/opensearch-project/OpenSearch/pull/13895)) - Add support for index level max slice count setting for concurrent segment search ([#15336](https://github.com/opensearch-project/OpenSearch/pull/15336)) +- Add support to upload snapshot shard blobs with hashed prefix ([#15426](https://github.com/opensearch-project/OpenSearch/pull/15426)) ### Dependencies - Bump `netty` from 4.1.111.Final to 4.1.112.Final ([#15081](https://github.com/opensearch-project/OpenSearch/pull/15081)) diff --git a/server/src/main/java/org/opensearch/index/recovery/RemoteStoreRestoreService.java b/server/src/main/java/org/opensearch/index/recovery/RemoteStoreRestoreService.java index d3c6fc9d1f3bf..03d841d13b7f7 100644 --- a/server/src/main/java/org/opensearch/index/recovery/RemoteStoreRestoreService.java +++ b/server/src/main/java/org/opensearch/index/recovery/RemoteStoreRestoreService.java @@ -227,7 +227,8 @@ private RemoteRestoreResult executeRestore( .build(); } - IndexId indexId = new IndexId(indexName, updatedIndexMetadata.getIndexUUID()); + // This instance of IndexId is not related to Snapshot Restore. Hence, we are using the ctor without pathType. + IndexId indexId = new IndexId(indexName, updatedIndexMetadata.getIndexUUID(), IndexId.DEFAULT_SHARD_PATH_TYPE); if (metadataFromRemoteStore == false) { Map indexShardRoutingTableMap = currentState.routingTable() diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java b/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java index d5617bdfd94a7..5457fe80d6e2d 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java @@ -64,7 +64,8 @@ public class RemoteIndexPathUploader extends IndexMetadataUploadListener { public static final String DELIMITER = "#"; public static final ConfigBlobStoreFormat REMOTE_INDEX_PATH_FORMAT = new ConfigBlobStoreFormat<>( - RemoteIndexPath.FILE_NAME_FORMAT + RemoteIndexPath.FILE_NAME_FORMAT, + RemoteIndexPath::fromXContent ); private static final String TIMEOUT_EXCEPTION_MSG = "Timed out waiting while uploading remote index path file for indexes=%s"; diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java b/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java index b0376c97e6994..0af0d07e11597 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java @@ -207,7 +207,7 @@ public enum PathHashAlgorithm { @Override String hash(PathInput pathInput) { StringBuilder input = new StringBuilder(); - for (String path : pathInput.fixedSubPath().toArray()) { + for (String path : pathInput.hashPath().toArray()) { input.append(path); } long hash = FNV1a.hash64(input.toString()); @@ -222,7 +222,7 @@ String hash(PathInput pathInput) { @Override String hash(PathInput pathInput) { StringBuilder input = new StringBuilder(); - for (String path : pathInput.fixedSubPath().toArray()) { + for (String path : pathInput.hashPath().toArray()) { input.append(path); } long hash = FNV1a.hash64(input.toString()); diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java index d0250790068f7..cd8949288ae3b 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java @@ -17,6 +17,7 @@ import org.opensearch.index.remote.RemoteStoreEnums.DataType; import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; import org.opensearch.index.remote.RemoteStoreEnums.PathType; +import org.opensearch.repositories.blobstore.BlobStoreRepository; import java.util.Objects; @@ -100,6 +101,10 @@ BlobPath fixedSubPath() { return BlobPath.cleanPath().add(indexUUID); } + BlobPath hashPath() { + return fixedSubPath(); + } + /** * Returns a new builder for {@link PathInput}. */ @@ -127,7 +132,7 @@ public T basePath(BlobPath basePath) { return self(); } - public Builder indexUUID(String indexUUID) { + public T indexUUID(String indexUUID) { this.indexUUID = indexUUID; return self(); } @@ -142,6 +147,61 @@ public PathInput build() { } } + /** + * A subclass of {@link PathInput} that represents the input required to generate a path + * for a shard in a snapshot. It includes the base path, index UUID, and shard ID. + * + * @opensearch.internal + */ + public static class SnapshotShardPathInput extends PathInput { + private final String shardId; + + public SnapshotShardPathInput(SnapshotShardPathInput.Builder builder) { + super(builder); + this.shardId = Objects.requireNonNull(builder.shardId); + } + + @Override + BlobPath fixedSubPath() { + return BlobPath.cleanPath().add(BlobStoreRepository.INDICES_DIR).add(super.fixedSubPath()).add(shardId); + } + + @Override + BlobPath hashPath() { + return BlobPath.cleanPath().add(shardId).add(indexUUID()); + } + + /** + * Returns a new builder for {@link SnapshotShardPathInput}. + */ + public static SnapshotShardPathInput.Builder builder() { + return new SnapshotShardPathInput.Builder(); + } + + /** + * Builder for {@link SnapshotShardPathInput}. + * + * @opensearch.internal + */ + public static class Builder extends PathInput.Builder { + private String shardId; + + public SnapshotShardPathInput.Builder shardId(String shardId) { + this.shardId = shardId; + return this; + } + + @Override + protected SnapshotShardPathInput.Builder self() { + return this; + } + + public SnapshotShardPathInput build() { + return new SnapshotShardPathInput(this); + } + } + } + /** * Wrapper class for the data aware path input required to generate path for remote store uploads. This input is * composed of the parent inputs, shard id, data category and data type. @@ -204,16 +264,6 @@ public static class Builder extends PathInput.Builder { private DataCategory dataCategory; private DataType dataType; - public Builder basePath(BlobPath basePath) { - super.basePath = basePath; - return this; - } - - public Builder indexUUID(String indexUUID) { - super.indexUUID = indexUUID; - return this; - } - public Builder shardId(String shardId) { this.shardId = shardId; return this; diff --git a/server/src/main/java/org/opensearch/repositories/IndexId.java b/server/src/main/java/org/opensearch/repositories/IndexId.java index 87a0063e8c21b..238dffbb46bde 100644 --- a/server/src/main/java/org/opensearch/repositories/IndexId.java +++ b/server/src/main/java/org/opensearch/repositories/IndexId.java @@ -32,6 +32,7 @@ package org.opensearch.repositories; +import org.opensearch.Version; import org.opensearch.common.annotation.PublicApi; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; @@ -40,6 +41,7 @@ import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.ToXContentObject; import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.index.remote.RemoteStoreEnums; import java.io.IOException; import java.util.Objects; @@ -51,23 +53,36 @@ */ @PublicApi(since = "1.0.0") public final class IndexId implements Writeable, ToXContentObject { - protected static final String NAME = "name"; - protected static final String ID = "id"; + static final String NAME = "name"; + static final String ID = "id"; + static final String SHARD_PATH_TYPE = "shard_path_type"; + public static final int DEFAULT_SHARD_PATH_TYPE = RemoteStoreEnums.PathType.FIXED.getCode(); private final String name; private final String id; + private final int shardPathType; private final int hashCode; + // Used for testing only public IndexId(final String name, final String id) { + this(name, id, DEFAULT_SHARD_PATH_TYPE); + } + + public IndexId(String name, String id, int shardPathType) { this.name = name; this.id = id; + this.shardPathType = shardPathType; this.hashCode = computeHashCode(); - } public IndexId(final StreamInput in) throws IOException { this.name = in.readString(); this.id = in.readString(); + if (in.getVersion().onOrAfter(Version.CURRENT)) { + this.shardPathType = in.readVInt(); + } else { + this.shardPathType = DEFAULT_SHARD_PATH_TYPE; + } this.hashCode = computeHashCode(); } @@ -93,9 +108,16 @@ public String getId() { return id; } + /** + * The storage path type in remote store for the indexes having the underlying index ids. + */ + public int getShardPathType() { + return shardPathType; + } + @Override public String toString() { - return "[" + name + "/" + id + "]"; + return "[" + name + "/" + id + "/" + shardPathType + "]"; } @Override @@ -107,7 +129,7 @@ public boolean equals(Object o) { return false; } IndexId that = (IndexId) o; - return Objects.equals(name, that.name) && Objects.equals(id, that.id); + return Objects.equals(name, that.name) && Objects.equals(id, that.id) && Objects.equals(this.shardPathType, that.shardPathType); } @Override @@ -116,13 +138,16 @@ public int hashCode() { } private int computeHashCode() { - return Objects.hash(name, id); + return Objects.hash(name, id, shardPathType); } @Override public void writeTo(final StreamOutput out) throws IOException { out.writeString(name); out.writeString(id); + if (out.getVersion().onOrAfter(Version.CURRENT)) { + out.writeVInt(shardPathType); + } } @Override @@ -130,6 +155,7 @@ public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params par builder.startObject(); builder.field(NAME, name); builder.field(ID, id); + builder.field(SHARD_PATH_TYPE, shardPathType); builder.endObject(); return builder; } diff --git a/server/src/main/java/org/opensearch/repositories/RepositoryData.java b/server/src/main/java/org/opensearch/repositories/RepositoryData.java index ea48d9b1a49fe..09cb654604c06 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoryData.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoryData.java @@ -517,7 +517,7 @@ public List resolveIndices(final List indices) { * @param indicesToResolve names of indices to resolve * @param inFlightIds name to index mapping for currently in-flight snapshots not yet in the repository data to fall back to */ - public List resolveNewIndices(List indicesToResolve, Map inFlightIds) { + public List resolveNewIndices(List indicesToResolve, Map inFlightIds, int pathType) { List snapshotIndices = new ArrayList<>(); for (String index : indicesToResolve) { IndexId indexId = indices.get(index); @@ -525,7 +525,7 @@ public List resolveNewIndices(List indicesToResolve, Map resolveNewIndices(List indicesToResolve, Map snapshotIds = indexSnapshots.get(indexId); assert snapshotIds != null; @@ -765,14 +774,20 @@ private static void parseIndices( final List snapshotIds = new ArrayList<>(); final List gens = new ArrayList<>(); + String id = null; + int pathType = IndexId.DEFAULT_SHARD_PATH_TYPE; IndexId indexId = null; + XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); while (parser.nextToken() != XContentParser.Token.END_OBJECT) { final String indexMetaFieldName = parser.currentName(); final XContentParser.Token currentToken = parser.nextToken(); switch (indexMetaFieldName) { case INDEX_ID: - indexId = new IndexId(indexName, parser.text()); + id = parser.text(); + break; + case IndexId.SHARD_PATH_TYPE: + pathType = parser.intValue(); break; case SNAPSHOTS: XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_ARRAY, currentToken, parser); @@ -795,7 +810,7 @@ private static void parseIndices( // different versions create or delete snapshot in the same repository. throw new OpenSearchParseException( "Detected a corrupted repository, index " - + indexId + + new IndexId(indexName, id, pathType) + " references an unknown snapshot uuid [" + uuid + "]" @@ -812,9 +827,10 @@ private static void parseIndices( break; } } - assert indexId != null; + assert id != null; + indexId = new IndexId(indexName, id, pathType); indexSnapshots.put(indexId, Collections.unmodifiableList(snapshotIds)); - indexLookup.put(indexId.getId(), indexId); + indexLookup.put(id, indexId); for (int i = 0; i < gens.size(); i++) { String parsedGen = gens.get(i); if (parsedGen != null) { diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java index 262a32fa1e74d..524effdd41ad8 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java @@ -9,19 +9,34 @@ package org.opensearch.repositories.blobstore; import org.apache.lucene.codecs.CodecUtil; +import org.apache.lucene.index.CorruptIndexException; +import org.apache.lucene.index.IndexFormatTooNewException; +import org.apache.lucene.index.IndexFormatTooOldException; +import org.apache.lucene.store.ByteBuffersDataInput; +import org.apache.lucene.store.ByteBuffersIndexInput; +import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.OutputStreamIndexOutput; +import org.apache.lucene.util.BytesRef; +import org.opensearch.common.CheckedFunction; import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.io.stream.BytesStreamOutput; +import org.opensearch.common.lucene.store.ByteArrayIndexInput; import org.opensearch.common.lucene.store.IndexOutputOutputStream; +import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.compress.Compressor; import org.opensearch.core.xcontent.MediaTypeRegistry; +import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.gateway.CorruptStateException; import java.io.IOException; import java.io.OutputStream; +import java.util.Arrays; import java.util.Locale; import java.util.Objects; @@ -30,7 +45,7 @@ * * @opensearch.internal */ -public class BaseBlobStoreFormat { +public abstract class BaseBlobStoreFormat { private static final int BUFFER_SIZE = 4096; @@ -91,7 +106,7 @@ public BytesReference serialize( try (BytesStreamOutput outputStream = new BytesStreamOutput()) { try ( OutputStreamIndexOutput indexOutput = new OutputStreamIndexOutput( - "ChecksumBlobStoreFormat.writeBlob(blob=\"" + blobName + "\")", + "BaseBlobStoreFormat.writeBlob(blob=\"" + blobName + "\")", blobName, outputStream, BUFFER_SIZE @@ -127,4 +142,43 @@ public void close() { protected String getBlobNameFormat() { return blobNameFormat; } + + public T deserialize( + String blobName, + NamedXContentRegistry namedXContentRegistry, + BytesReference bytes, + XContentType xContentType, + String codec, + Integer version + ) throws IOException { + assert skipHeaderFooter || (Objects.nonNull(codec) && Objects.nonNull(version)); + final String resourceDesc = "BaseBlobStoreFormat.readBlob(blob=\"" + blobName + "\")"; + try { + final IndexInput indexInput = bytes.length() > 0 + ? new ByteBuffersIndexInput(new ByteBuffersDataInput(Arrays.asList(BytesReference.toByteBuffers(bytes))), resourceDesc) + : new ByteArrayIndexInput(resourceDesc, BytesRef.EMPTY_BYTES); + if (skipHeaderFooter == false) { + CodecUtil.checksumEntireFile(indexInput); + CodecUtil.checkHeader(indexInput, codec, version, version); + } + int footerLength = skipHeaderFooter ? 0 : CodecUtil.footerLength(); + long filePointer = indexInput.getFilePointer(); + long contentSize = indexInput.length() - footerLength - filePointer; + try ( + XContentParser parser = XContentHelper.createParser( + namedXContentRegistry, + LoggingDeprecationHandler.INSTANCE, + bytes.slice((int) filePointer, (int) contentSize), + xContentType + ) + ) { + return reader().apply(parser); + } + } catch (CorruptIndexException | IndexFormatTooOldException | IndexFormatTooNewException ex) { + // we trick this into a dedicated exception with the original stacktrace + throw new CorruptStateException(ex); + } + } + + abstract CheckedFunction reader(); } 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 3e6a75565891f..f40f43b8b8d90 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -108,8 +108,11 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.mapper.MapperService; +import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.index.remote.RemoteStorePathStrategy; import org.opensearch.index.remote.RemoteStorePathStrategy.PathInput; +import org.opensearch.index.remote.RemoteStorePathStrategy.SnapshotShardPathInput; import org.opensearch.index.snapshots.IndexShardRestoreFailedException; import org.opensearch.index.snapshots.IndexShardSnapshotStatus; import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot; @@ -144,6 +147,7 @@ import org.opensearch.snapshots.SnapshotId; import org.opensearch.snapshots.SnapshotInfo; import org.opensearch.snapshots.SnapshotMissingException; +import org.opensearch.snapshots.SnapshotShardPaths; import org.opensearch.snapshots.SnapshotsService; import org.opensearch.threadpool.ThreadPool; @@ -155,6 +159,8 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; @@ -177,7 +183,6 @@ import java.util.stream.Stream; import static org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm.FNV_1A_COMPOSITE_1; -import static org.opensearch.index.remote.RemoteStoreEnums.PathType.HASHED_PREFIX; import static org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot.FileInfo.canonicalName; import static org.opensearch.repositories.blobstore.ChecksumBlobStoreFormat.SNAPSHOT_ONLY_FORMAT_PARAMS; @@ -223,6 +228,8 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp private static final String UPLOADED_DATA_BLOB_PREFIX = "__"; + public static final String INDICES_DIR = "indices"; + /** * Prefix used for the identifiers of data blobs that were not actually written to the repository physically because their contents are * already stored in the metadata referencing them, i.e. in {@link BlobStoreIndexShardSnapshot} and @@ -266,6 +273,12 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp public static final Setting REMOTE_STORE_INDEX_SHALLOW_COPY = Setting.boolSetting("remote_store_index_shallow_copy", false); + public static final Setting SHARD_PATH_TYPE = new Setting<>( + "shard_path_type", + PathType.FIXED.toString(), + PathType::parseString + ); + /** * Setting to set batch size of stale snapshot shard blobs that will be deleted by snapshot workers as part of snapshot deletion. * For optimal performance the value of the setting should be equal to or close to repository's max # of keys that can be deleted in single operation @@ -379,6 +392,11 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp BlobStoreIndexShardSnapshots::fromXContent ); + public static final ConfigBlobStoreFormat SNAPSHOT_SHARD_PATHS_FORMAT = new ConfigBlobStoreFormat<>( + SnapshotShardPaths.FILE_NAME_FORMAT, + SnapshotShardPaths::fromXContent + ); + private volatile boolean readOnly; private final boolean isSystemRepository; @@ -389,6 +407,10 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp private final SetOnce blobContainer = new SetOnce<>(); + private final SetOnce rootBlobContainer = new SetOnce<>(); + + private final SetOnce snapshotShardPathBlobContainer = new SetOnce<>(); + private final SetOnce blobStore = new SetOnce<>(); protected final ClusterService clusterService; @@ -779,6 +801,16 @@ BlobContainer getBlobContainer() { return blobContainer.get(); } + // package private, only use for testing + BlobContainer getRootBlobContainer() { + return rootBlobContainer.get(); + } + + // package private, only use for testing + public SetOnce getSnapshotShardPathBlobContainer() { + return snapshotShardPathBlobContainer; + } + // for test purposes only protected BlobStore getBlobStore() { return blobStore.get(); @@ -804,10 +836,47 @@ protected BlobContainer blobContainer() { } } } - return blobContainer; } + /** + * maintains single lazy instance of {@link BlobContainer} + */ + protected BlobContainer rootBlobContainer() { + assertSnapshotOrGenericThread(); + + BlobContainer rootBlobContainer = this.rootBlobContainer.get(); + if (rootBlobContainer == null) { + synchronized (lock) { + rootBlobContainer = this.rootBlobContainer.get(); + if (rootBlobContainer == null) { + rootBlobContainer = blobStore().blobContainer(BlobPath.cleanPath()); + this.rootBlobContainer.set(rootBlobContainer); + } + } + } + return rootBlobContainer; + } + + /** + * maintains single lazy instance of {@link BlobContainer} + */ + protected BlobContainer snapshotShardPathBlobContainer() { + assertSnapshotOrGenericThread(); + + BlobContainer snapshotShardPathBlobContainer = this.snapshotShardPathBlobContainer.get(); + if (snapshotShardPathBlobContainer == null) { + synchronized (lock) { + snapshotShardPathBlobContainer = this.snapshotShardPathBlobContainer.get(); + if (snapshotShardPathBlobContainer == null) { + snapshotShardPathBlobContainer = blobStore().blobContainer(basePath().add(SnapshotShardPaths.DIR)); + this.snapshotShardPathBlobContainer.set(snapshotShardPathBlobContainer); + } + } + } + return snapshotShardPathBlobContainer; + } + /** * Maintains single lazy instance of {@link BlobStore}. * Public for testing. @@ -1099,7 +1168,7 @@ private void asyncCleanupUnlinkedShardLevelBlobs( RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, ActionListener listener ) { - final List filesToDelete = resolveFilesToDelete(oldRepositoryData, snapshotIds, deleteResults); + final List> filesToDelete = resolveFilesToDelete(oldRepositoryData, snapshotIds, deleteResults); if (filesToDelete.isEmpty()) { listener.onResponse(null); return; @@ -1107,10 +1176,10 @@ private void asyncCleanupUnlinkedShardLevelBlobs( try { AtomicInteger counter = new AtomicInteger(); - Collection> subList = filesToDelete.stream() + Collection>> subList = filesToDelete.stream() .collect(Collectors.groupingBy(it -> counter.getAndIncrement() / maxShardBlobDeleteBatch)) .values(); - final BlockingQueue> staleFilesToDeleteInBatch = new LinkedBlockingQueue<>(subList); + final BlockingQueue>> staleFilesToDeleteInBatch = new LinkedBlockingQueue<>(subList); final GroupedActionListener groupedListener = new GroupedActionListener<>( ActionListener.wrap(r -> { listener.onResponse(null); }, listener::onFailure), @@ -1213,57 +1282,64 @@ protected void releaseRemoteStoreLockAndCleanup( // When remoteStoreLockManagerFactory is non-null, while deleting the files, lock files are also released before deletion of respective // shallow-snap-UUID files. And if it is null, we just delete the stale shard blobs. private void executeStaleShardDelete( - BlockingQueue> staleFilesToDeleteInBatch, + BlockingQueue>> staleFilesToDeleteInBatch, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, GroupedActionListener listener ) throws InterruptedException { - List filesToDelete = staleFilesToDeleteInBatch.poll(0L, TimeUnit.MILLISECONDS); - if (filesToDelete != null) { - threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(ActionRunnable.wrap(listener, l -> { - try { - // filtering files for which remote store lock release and cleanup succeeded, - // remaining files for which it failed will be retried in next snapshot delete run. - List eligibleFilesToDelete = new ArrayList<>(); - for (String fileToDelete : filesToDelete) { - if (fileToDelete.contains(SHALLOW_SNAPSHOT_PREFIX)) { - String[] fileToDeletePath = fileToDelete.split("/"); - String indexId = fileToDeletePath[1]; - String shardId = fileToDeletePath[2]; - String shallowSnapBlob = fileToDeletePath[3]; - String snapshotUUID = extractShallowSnapshotUUID(shallowSnapBlob).orElseThrow(); - BlobContainer shardContainer = blobStore().blobContainer(indicesPath().add(indexId).add(shardId)); - try { - releaseRemoteStoreLockAndCleanup(shardId, snapshotUUID, shardContainer, remoteStoreLockManagerFactory); - eligibleFilesToDelete.add(fileToDelete); - } catch (Exception e) { - logger.error( - "Failed to release lock or cleanup shard for indexID {}, shardID {} " + "and snapshot {}", - indexId, - shardId, - snapshotUUID - ); - } - } else { - eligibleFilesToDelete.add(fileToDelete); + List> filesToDelete = staleFilesToDeleteInBatch.poll(0L, TimeUnit.MILLISECONDS); + if (filesToDelete == null) { + return; + } + threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(ActionRunnable.wrap(listener, l -> { + try { + // filtering files for which remote store lock release and cleanup succeeded, + // remaining files for which it failed will be retried in next snapshot delete run. + List eligibleFilesToDelete = new ArrayList<>(); + for (Tuple fileToDelete : filesToDelete) { + BlobPath blobPath = fileToDelete.v1(); + String blobName = fileToDelete.v2(); + boolean deleteBlob = false; + if (blobName.startsWith(SHALLOW_SNAPSHOT_PREFIX)) { + String snapshotUUID = extractShallowSnapshotUUID(blobName).orElseThrow(); + String[] parts = blobPath.toArray(); + int partLength = parts.length; + String indexId = parts[partLength - 2]; + String shardId = parts[partLength - 1]; + BlobContainer shardContainer = blobStore().blobContainer(blobPath); + try { + releaseRemoteStoreLockAndCleanup(shardId, snapshotUUID, shardContainer, remoteStoreLockManagerFactory); + deleteBlob = true; + } catch (Exception e) { + logger.error( + "Failed to release lock or cleanup shard for indexID {}, shardID {} and snapshot {}", + indexId, + shardId, + snapshotUUID + ); } + } else { + deleteBlob = true; + } + if (deleteBlob) { + eligibleFilesToDelete.add(blobPath.buildAsString() + blobName); } - // Deleting the shard blobs - deleteFromContainer(blobContainer(), eligibleFilesToDelete); - l.onResponse(null); - } catch (Exception e) { - logger.warn( - () -> new ParameterizedMessage( - "[{}] Failed to delete following blobs during snapshot delete : {}", - metadata.name(), - filesToDelete - ), - e - ); - l.onFailure(e); } - executeStaleShardDelete(staleFilesToDeleteInBatch, remoteStoreLockManagerFactory, listener); - })); - } + // Deleting the shard blobs + deleteFromContainer(rootBlobContainer(), eligibleFilesToDelete); + l.onResponse(null); + } catch (Exception e) { + logger.warn( + () -> new ParameterizedMessage( + "[{}] Failed to delete following blobs during snapshot delete : {}", + metadata.name(), + filesToDelete + ), + e + ); + l.onFailure(e); + } + executeStaleShardDelete(staleFilesToDeleteInBatch, remoteStoreLockManagerFactory, listener); + })); } // updates the shard state metadata for shards of a snapshot that is to be deleted. Also computes the files to be cleaned up. @@ -1405,26 +1481,30 @@ public void onFailure(Exception ex) { } } - private List resolveFilesToDelete( + /** + * Resolves the list of files that should be deleted during a snapshot deletion operation. + * This method combines files to be deleted from shard-level metadata and index-level metadata. + * + * @param oldRepositoryData The repository data before the snapshot deletion + * @param snapshotIds The IDs of the snapshots being deleted + * @param deleteResults The results of removing snapshots from shard-level metadata + * @return A list of tuples, each containing a blob path and the name of a blob to be deleted + */ + private List> resolveFilesToDelete( RepositoryData oldRepositoryData, Collection snapshotIds, Collection deleteResults ) { - final String basePath = basePath().buildAsString(); - final int basePathLen = basePath.length(); final Map> indexMetaGenerations = oldRepositoryData.indexMetaDataToRemoveAfterRemovingSnapshots( snapshotIds ); return Stream.concat(deleteResults.stream().flatMap(shardResult -> { - final String shardPath = shardContainer(shardResult.indexId, shardResult.shardId).path().buildAsString(); - return shardResult.blobsToDelete.stream().map(blob -> shardPath + blob); + final BlobPath shardPath = shardPath(shardResult.indexId, shardResult.shardId); + return shardResult.blobsToDelete.stream().map(blob -> Tuple.tuple(shardPath, blob)); }), indexMetaGenerations.entrySet().stream().flatMap(entry -> { - final String indexContainerPath = indexContainer(entry.getKey()).path().buildAsString(); - return entry.getValue().stream().map(id -> indexContainerPath + INDEX_METADATA_FORMAT.blobName(id)); - })).map(absolutePath -> { - assert absolutePath.startsWith(basePath); - return absolutePath.substring(basePathLen); - }).collect(Collectors.toList()); + final BlobPath indexPath = indexPath(entry.getKey()); + return entry.getValue().stream().map(id -> Tuple.tuple(indexPath, INDEX_METADATA_FORMAT.blobName(id))); + })).collect(Collectors.toList()); } /** @@ -1472,10 +1552,20 @@ private void cleanupStaleBlobs( if (foundIndices.keySet().equals(survivingIndexIds)) { groupedListener.onResponse(DeleteResult.ZERO); } else { - cleanupStaleIndices(foundIndices, survivingIndexIds, remoteStoreLockManagerFactory, groupedListener); + Map snapshotShardPaths = getSnapshotShardPaths(); + cleanupStaleIndices(foundIndices, survivingIndexIds, remoteStoreLockManagerFactory, groupedListener, snapshotShardPaths); } } + private Map getSnapshotShardPaths() { + try { + return snapshotShardPathBlobContainer().listBlobs(); + } catch (IOException ex) { + logger.warn(new ParameterizedMessage("Repository [{}] Failed to get the snapshot shard paths", metadata.name()), ex); + } + return Collections.emptyMap(); + } + /** * Runs cleanup actions on the repository. Increments the repository state id by one before executing any modifications on the * repository. If remoteStoreLockManagerFactory is not null, remote store lock files are released when deleting the respective @@ -1619,7 +1709,8 @@ private void cleanupStaleIndices( Map foundIndices, Set survivingIndexIds, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, - GroupedActionListener listener + GroupedActionListener listener, + Map snapshotShardPaths ) { final GroupedActionListener groupedListener = new GroupedActionListener<>(ActionListener.wrap(deleteResults -> { DeleteResult deleteResult = DeleteResult.ZERO; @@ -1643,7 +1734,7 @@ private void cleanupStaleIndices( foundIndices.size() - survivingIndexIds.size() ); for (int i = 0; i < workers; ++i) { - executeOneStaleIndexDelete(staleIndicesToDelete, remoteStoreLockManagerFactory, groupedListener); + executeOneStaleIndexDelete(staleIndicesToDelete, remoteStoreLockManagerFactory, groupedListener, snapshotShardPaths); } } catch (Exception e) { // TODO: We shouldn't be blanket catching and suppressing all exceptions here and instead handle them safely upstream. @@ -1663,55 +1754,185 @@ private static boolean isIndexPresent(ClusterService clusterService, String inde return false; } + /** + * Executes the deletion of a single stale index. + * + * @param staleIndicesToDelete Queue of stale indices to delete + * @param remoteStoreLockManagerFactory Factory for creating remote store lock managers + * @param listener Listener for grouped delete actions + * @param snapshotShardPaths Map of snapshot shard paths and their metadata + * @throws InterruptedException if the thread is interrupted while waiting + */ private void executeOneStaleIndexDelete( BlockingQueue> staleIndicesToDelete, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, - GroupedActionListener listener + GroupedActionListener listener, + Map snapshotShardPaths ) throws InterruptedException { Map.Entry indexEntry = staleIndicesToDelete.poll(0L, TimeUnit.MILLISECONDS); - if (indexEntry != null) { - final String indexSnId = indexEntry.getKey(); - threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(ActionRunnable.supply(listener, () -> { - DeleteResult deleteResult = DeleteResult.ZERO; - try { - logger.debug("[{}] Found stale index [{}]. Cleaning it up", metadata.name(), indexSnId); - if (remoteStoreLockManagerFactory != null) { - final Map shardBlobs = indexEntry.getValue().children(); - for (Map.Entry shardBlob : shardBlobs.entrySet()) { - for (String blob : shardBlob.getValue().listBlobs().keySet()) { - final Optional snapshotUUID = extractShallowSnapshotUUID(blob); - if (snapshotUUID.isPresent()) { - releaseRemoteStoreLockAndCleanup( - shardBlob.getKey(), - snapshotUUID.get(), - shardBlob.getValue(), - remoteStoreLockManagerFactory - ); - } - } - } - } - // Deleting the index folder - deleteResult = indexEntry.getValue().delete(); - logger.debug("[{}] Cleaned up stale index [{}]", metadata.name(), indexSnId); - } catch (IOException e) { - logger.warn( - () -> new ParameterizedMessage( - "[{}] index {} is no longer part of any snapshots in the repository, " - + "but failed to clean up their index folders", - metadata.name(), - indexSnId - ), - e - ); - } catch (Exception e) { - assert false : e; - logger.warn(new ParameterizedMessage("[{}] Exception during single stale index delete", metadata.name()), e); + if (indexEntry == null) { + return; + } + final String indexSnId = indexEntry.getKey(); + threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(ActionRunnable.supply(listener, () -> { + try { + logger.debug("[{}] Found stale index [{}]. Cleaning it up", metadata.name(), indexSnId); + List matchingShardPaths = findMatchingShardPaths(indexSnId, snapshotShardPaths); + Optional highestGenShardPaths = findHighestGenerationShardPaths(matchingShardPaths); + + if (remoteStoreLockManagerFactory != null) { + cleanupRemoteStoreLocks(indexEntry, highestGenShardPaths, remoteStoreLockManagerFactory); } - executeOneStaleIndexDelete(staleIndicesToDelete, remoteStoreLockManagerFactory, listener); + DeleteResult deleteResult = deleteShardData(highestGenShardPaths, matchingShardPaths, snapshotShardPaths); + deleteResult = deleteResult.add(indexEntry.getValue().delete()); // Deleting the index folder + logger.debug("[{}] Cleaned up stale index [{}]", metadata.name(), indexSnId); return deleteResult; - })); + } catch (IOException e) { + logger.warn( + () -> new ParameterizedMessage( + "[{}] index {} is no longer part of any snapshots in the repository, " + + "but failed to clean up their index folders", + metadata.name(), + indexSnId + ), + e + ); + return DeleteResult.ZERO; + } catch (Exception e) { + assert false : e; + logger.warn(new ParameterizedMessage("[{}] Exception during single stale index delete", metadata.name()), e); + return DeleteResult.ZERO; + } finally { + executeOneStaleIndexDelete(staleIndicesToDelete, remoteStoreLockManagerFactory, listener, snapshotShardPaths); + } + })); + } + + /** + * Finds and returns a list of shard paths that match the given index ID. + * + * @param indexId The ID of the index to match + * @param snapshotShardPaths Map of snapshot shard paths and their metadata + * @return List of matching shard paths + */ + private List findMatchingShardPaths(String indexId, Map snapshotShardPaths) { + return snapshotShardPaths.keySet().stream().filter(s -> s.startsWith(indexId)).collect(Collectors.toList()); + } + + /** + * Finds the shard path with the highest generation number from the given list of matching shard paths. + * + * @param matchingShardPaths List of shard paths that match a specific criteria + * @return An Optional containing the shard path with the highest generation number, or empty if the list is empty + */ + private Optional findHighestGenerationShardPaths(List matchingShardPaths) { + return matchingShardPaths.stream() + .map(s -> s.split(SnapshotShardPaths.DELIMITER)) + .sorted((a, b) -> Integer.parseInt(b[2]) - Integer.parseInt(a[2])) + .map(parts -> String.join(SnapshotShardPaths.DELIMITER, parts)) + .findFirst(); + } + + /** + * Cleans up remote store locks for a given index entry. + * + * @param indexEntry The index entry containing the blob container + * @param highestGenShardPaths The optional highest generation shard path + * @param remoteStoreLockManagerFactory Factory for creating remote store lock managers + * @throws IOException If an I/O error occurs during the cleanup process + */ + private void cleanupRemoteStoreLocks( + Map.Entry indexEntry, + Optional highestGenShardPaths, + RemoteStoreLockManagerFactory remoteStoreLockManagerFactory + ) throws IOException { + if (highestGenShardPaths.isEmpty()) { + releaseRemoteStoreLocksAndCleanup(indexEntry.getValue().children(), remoteStoreLockManagerFactory); + } else { + ShardInfo shardInfo = parseShardPath(highestGenShardPaths.get()); + Map shardContainers = new HashMap<>(shardInfo.shardCount); + for (int i = 0; i < shardInfo.shardCount; i++) { + shardContainers.put(String.valueOf(i), shardContainer(shardInfo.indexId, i)); + } + releaseRemoteStoreLocksAndCleanup(shardContainers, remoteStoreLockManagerFactory); + } + } + + /** + * Releases remote store locks and performs cleanup for each shard blob. + * + * @param shardBlobs Map of shard IDs to their corresponding BlobContainers + * @param remoteStoreLockManagerFactory Factory for creating remote store lock managers + * @throws IOException If an I/O error occurs during the release and cleanup process + */ + private void releaseRemoteStoreLocksAndCleanup( + Map shardBlobs, + RemoteStoreLockManagerFactory remoteStoreLockManagerFactory + ) throws IOException { + for (Map.Entry shardBlob : shardBlobs.entrySet()) { + for (String blob : shardBlob.getValue().listBlobs().keySet()) { + final Optional snapshotUUID = extractShallowSnapshotUUID(blob); + if (snapshotUUID.isPresent()) { + releaseRemoteStoreLockAndCleanup( + shardBlob.getKey(), + snapshotUUID.get(), + shardBlob.getValue(), + remoteStoreLockManagerFactory + ); + } + } + } + } + + /** + * Deletes shard data for the highest generation shard paths. + * + * @param highestGenShardPaths The optional highest generation shard path + * @param matchingShardPaths List of shard paths that match a specific criteria + * @param snapshotShardPaths Map of snapshot shard paths and their metadata + * @throws IOException If an I/O error occurs during the deletion process + */ + private DeleteResult deleteShardData( + Optional highestGenShardPaths, + List matchingShardPaths, + Map snapshotShardPaths + ) throws IOException { + if (highestGenShardPaths.isEmpty()) { + return DeleteResult.ZERO; + } + + ShardInfo shardInfo = parseShardPath(highestGenShardPaths.get()); + DeleteResult deleteResult = DeleteResult.ZERO; + + for (int i = 0; i < shardInfo.shardCount; i++) { + deleteResult = deleteResult.add(shardContainer(shardInfo.indexId, i).delete()); + } + + deleteFromContainer(snapshotShardPathBlobContainer(), matchingShardPaths); + long totalBytes = matchingShardPaths.stream().mapToLong(s -> snapshotShardPaths.get(s).length()).sum(); + return deleteResult.add(matchingShardPaths.size(), totalBytes); + } + + /** + * Parses a shard path string and extracts relevant shard information. + * + * @param shardPath The shard path string to parse. Expected format is: + * [index_id]#[index_name]#[shard_count]#[path_type_code]#[path_hash_algorithm_code] + * @return A {@link ShardInfo} object containing the parsed index ID and shard count. + * @throws IllegalArgumentException if the shard path format is invalid or cannot be parsed. + */ + private ShardInfo parseShardPath(String shardPath) { + String[] parts = shardPath.split(SnapshotShardPaths.DELIMITER); + if (parts.length != 5) { + throw new IllegalArgumentException("Invalid shard path format: " + shardPath); + } + try { + IndexId indexId = new IndexId(parts[1], parts[0], Integer.parseInt(parts[3])); + int shardCount = Integer.parseInt(parts[2]); + return new ShardInfo(indexId, shardCount); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid shard path format: " + shardPath, e); } } @@ -1754,6 +1975,8 @@ public void finalizeSnapshot( indexMetas, indexMetaIdentifiers ); + Set updatedIndexIds = writeNewIndexShardPaths(existingRepositoryData, updatedRepositoryData, snapshotId); + cleanupStaleSnapshotShardPaths(updatedIndexIds); writeIndexGen( updatedRepositoryData, repositoryStateId, @@ -1804,21 +2027,130 @@ public void finalizeSnapshot( }, onUpdateFailure); } + private void cleanupStaleSnapshotShardPaths(Set updatedShardPathsIndexIds) { + Set updatedIndexIds = updatedShardPathsIndexIds.stream() + .map(s -> s.split(SnapshotShardPaths.DELIMITER)[0]) + .collect(Collectors.toSet()); + Set indexIdShardPaths = getSnapshotShardPaths().keySet(); + List staleShardPaths = indexIdShardPaths.stream().filter(s -> updatedShardPathsIndexIds.contains(s) == false).filter(s -> { + String indexId = s.split(SnapshotShardPaths.DELIMITER)[0]; + return updatedIndexIds.contains(indexId); + }).collect(Collectors.toList()); + try { + deleteFromContainer(snapshotShardPathBlobContainer(), staleShardPaths); + } catch (IOException e) { + logger.warn( + new ParameterizedMessage( + "Repository [{}] Exception during snapshot stale index deletion {}", + metadata.name(), + staleShardPaths + ), + e + ); + } + } + + private Set writeNewIndexShardPaths( + RepositoryData existingRepositoryData, + RepositoryData updatedRepositoryData, + SnapshotId snapshotId + ) { + Set updatedIndexIds = new HashSet<>(); + Set indicesToUpdate = new HashSet<>(updatedRepositoryData.getIndices().values()); + for (IndexId indexId : indicesToUpdate) { + if (indexId.getShardPathType() == PathType.FIXED.getCode()) { + continue; + } + int oldShardCount = existingRepositoryData.shardGenerations().getGens(indexId).size(); + int newShardCount = updatedRepositoryData.shardGenerations().getGens(indexId).size(); + if (newShardCount > oldShardCount) { + String shardPathsBlobName = writeIndexShardPaths(indexId, snapshotId, newShardCount); + if (Objects.nonNull(shardPathsBlobName)) { + updatedIndexIds.add(shardPathsBlobName); + } + } + } + return updatedIndexIds; + } + + private String writeIndexShardPaths(IndexId indexId, SnapshotId snapshotId, int shardCount) { + try { + List paths = getShardPaths(indexId, shardCount); + String pathType = String.valueOf(indexId.getShardPathType()); + String pathHashAlgorithm = String.valueOf(FNV_1A_COMPOSITE_1.getCode()); + String blobName = String.join( + SnapshotShardPaths.DELIMITER, + indexId.getId(), + indexId.getName(), + String.valueOf(shardCount), + pathType, + pathHashAlgorithm + ); + SnapshotShardPaths shardPaths = new SnapshotShardPaths(paths); + SNAPSHOT_SHARD_PATHS_FORMAT.writeAsyncWithUrgentPriority( + shardPaths, + snapshotShardPathBlobContainer(), + blobName, + getSnapshotShardListener(indexId, snapshotId) + ); + return blobName; + } catch (IOException e) { + logShardPathsOperationWarning(indexId, snapshotId, e); + } + return null; + } + + private List getShardPaths(IndexId indexId, int shardCount) { + List paths = new ArrayList<>(); + for (int shardId = 0; shardId < shardCount; shardId++) { + BlobPath shardPath = shardPath(indexId, shardId); + paths.add(shardPath.buildAsString()); + } + return paths; + } + + private ActionListener getSnapshotShardListener(IndexId indexId, SnapshotId snapshotId) { + return ActionListener.wrap( + resp -> logShardPathsOperationSuccess(indexId, snapshotId), + ex -> logShardPathsOperationWarning(indexId, snapshotId, ex) + ); + } + + private void logShardPathsOperationSuccess(IndexId indexId, SnapshotId snapshotId) { + logger.trace( + () -> new ParameterizedMessage( + "Repository [{}] successfully wrote shard paths for index [{}] in snapshot [{}]", + metadata.name(), + indexId.getName(), + snapshotId.getName() + ) + ); + } + + private void logShardPathsOperationWarning(IndexId indexId, SnapshotId snapshotId, @Nullable Exception e) { + logger.warn( + () -> new ParameterizedMessage( + "Repository [{}] Failed to write shard paths for index [{}] in snapshot [{}]", + metadata.name(), + indexId.getName(), + snapshotId.getName() + ), + e + ); + } + // Delete all old shard gen blobs that aren't referenced any longer as a result from moving to updated repository data private void cleanupOldShardGens(RepositoryData existingRepositoryData, RepositoryData updatedRepositoryData) { final List toDelete = new ArrayList<>(); - final int prefixPathLen = basePath().buildAsString().length(); updatedRepositoryData.shardGenerations() .obsoleteShardGenerations(existingRepositoryData.shardGenerations()) .forEach( (indexId, gens) -> gens.forEach( - (shardId, oldGen) -> toDelete.add( - shardContainer(indexId, shardId).path().buildAsString().substring(prefixPathLen) + INDEX_FILE_PREFIX + oldGen - ) + (shardId, oldGen) -> toDelete.add(shardPath(indexId, shardId).buildAsString() + INDEX_FILE_PREFIX + oldGen) ) ); try { - deleteFromContainer(blobContainer(), toDelete); + deleteFromContainer(rootBlobContainer(), toDelete); } catch (Exception e) { logger.warn("Failed to clean up old shard generation blobs", e); } @@ -1865,11 +2197,15 @@ private void deleteFromContainer(BlobContainer container, List blobs) th } private BlobPath indicesPath() { - return basePath().add("indices"); + return basePath().add(INDICES_DIR); } private BlobContainer indexContainer(IndexId indexId) { - return blobStore().blobContainer(indicesPath().add(indexId.getId())); + return blobStore().blobContainer(indexPath(indexId)); + } + + private BlobPath indexPath(IndexId indexId) { + return indicesPath().add(indexId.getId()); } private BlobContainer shardContainer(IndexId indexId, ShardId shardId) { @@ -1877,7 +2213,17 @@ private BlobContainer shardContainer(IndexId indexId, ShardId shardId) { } public BlobContainer shardContainer(IndexId indexId, int shardId) { - return blobStore().blobContainer(indicesPath().add(indexId.getId()).add(Integer.toString(shardId))); + return blobStore().blobContainer(shardPath(indexId, shardId)); + } + + private BlobPath shardPath(IndexId indexId, int shardId) { + PathType pathType = PathType.fromCode(indexId.getShardPathType()); + SnapshotShardPathInput shardPathInput = new SnapshotShardPathInput.Builder().basePath(basePath()) + .indexUUID(indexId.getId()) + .shardId(String.valueOf(shardId)) + .build(); + PathHashAlgorithm pathHashAlgorithm = pathType != PathType.FIXED ? FNV_1A_COMPOSITE_1 : null; + return pathType.path(shardPathInput, pathHashAlgorithm); } /** @@ -1964,7 +2310,7 @@ private BlobContainer testContainer(String seed) { BlobPath testBlobPath; if (prefixModeVerification == true) { PathInput pathInput = PathInput.builder().basePath(basePath()).indexUUID(seed).build(); - testBlobPath = HASHED_PREFIX.path(pathInput, FNV_1A_COMPOSITE_1); + testBlobPath = PathType.HASHED_PREFIX.path(pathInput, FNV_1A_COMPOSITE_1); } else { testBlobPath = basePath(); } @@ -2052,11 +2398,12 @@ private void doGetRepositoryData(ActionListener listener) { loaded = repositoryDataFromCachedEntry(cached); } else { loaded = getRepositoryData(genToLoad); + Version minNodeVersion = clusterService.state().nodes().getMinNodeVersion(); // We can cache serialized in the most recent version here without regard to the actual repository metadata version // since we're only caching the information that we just wrote and thus won't accidentally cache any information that // isn't safe cacheRepositoryData( - BytesReference.bytes(loaded.snapshotsToXContent(XContentFactory.jsonBuilder(), Version.CURRENT)), + BytesReference.bytes(loaded.snapshotsToXContent(XContentFactory.jsonBuilder(), Version.CURRENT, minNodeVersion)), genToLoad ); } @@ -2444,8 +2791,9 @@ public void onFailure(Exception e) { } final String indexBlob = INDEX_FILE_PREFIX + Long.toString(newGen); logger.debug("Repository [{}] writing new index generational blob [{}]", metadata.name(), indexBlob); + Version minNodeVersion = clusterService.state().nodes().getMinNodeVersion(); final BytesReference serializedRepoData = BytesReference.bytes( - newRepositoryData.snapshotsToXContent(XContentFactory.jsonBuilder(), version) + newRepositoryData.snapshotsToXContent(XContentFactory.jsonBuilder(), version, minNodeVersion) ); writeAtomic(blobContainer(), indexBlob, serializedRepoData, true); maybeWriteIndexLatest(newGen); @@ -3655,4 +4003,27 @@ public String toString() { return name; } } + + /** + * Represents parsed information from a shard path. + * This class encapsulates the index ID and shard count extracted from a shard path string. + */ + private static class ShardInfo { + /** The ID of the index associated with this shard. */ + final IndexId indexId; + + /** The total number of shards for this index. */ + final int shardCount; + + /** + * Constructs a new ShardInfo instance. + * + * @param indexId The ID of the index associated with this shard. + * @param shardCount The total number of shards for this index. + */ + ShardInfo(IndexId indexId, int shardCount) { + this.indexId = indexId; + this.shardCount = shardCount; + } + } } diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java index 3a49fed4be282..e6f2218086e78 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java @@ -31,14 +31,7 @@ package org.opensearch.repositories.blobstore; -import org.apache.lucene.codecs.CodecUtil; -import org.apache.lucene.index.CorruptIndexException; -import org.apache.lucene.index.IndexFormatTooNewException; -import org.apache.lucene.index.IndexFormatTooOldException; -import org.apache.lucene.store.ByteBuffersDataInput; -import org.apache.lucene.store.ByteBuffersIndexInput; import org.apache.lucene.store.IndexInput; -import org.apache.lucene.util.BytesRef; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.common.CheckedFunction; import org.opensearch.common.blobstore.AsyncMultiStreamBlobContainer; @@ -48,8 +41,6 @@ import org.opensearch.common.blobstore.transfer.stream.OffsetRangeIndexInputStream; import org.opensearch.common.io.Streams; import org.opensearch.common.lucene.store.ByteArrayIndexInput; -import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.bytes.BytesReference; @@ -57,12 +48,10 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentParser; -import org.opensearch.gateway.CorruptStateException; import org.opensearch.index.store.exception.ChecksumCombinationException; import org.opensearch.snapshots.SnapshotInfo; import java.io.IOException; -import java.util.Arrays; import java.util.HashMap; import java.util.Locale; import java.util.Map; @@ -124,29 +113,7 @@ public String blobName(String name) { } public T deserialize(String blobName, NamedXContentRegistry namedXContentRegistry, BytesReference bytes) throws IOException { - final String resourceDesc = "ChecksumBlobStoreFormat.readBlob(blob=\"" + blobName + "\")"; - try { - final IndexInput indexInput = bytes.length() > 0 - ? new ByteBuffersIndexInput(new ByteBuffersDataInput(Arrays.asList(BytesReference.toByteBuffers(bytes))), resourceDesc) - : new ByteArrayIndexInput(resourceDesc, BytesRef.EMPTY_BYTES); - CodecUtil.checksumEntireFile(indexInput); - CodecUtil.checkHeader(indexInput, codec, VERSION, VERSION); - long filePointer = indexInput.getFilePointer(); - long contentSize = indexInput.length() - CodecUtil.footerLength() - filePointer; - try ( - XContentParser parser = XContentHelper.createParser( - namedXContentRegistry, - LoggingDeprecationHandler.INSTANCE, - bytes.slice((int) filePointer, (int) contentSize), - XContentType.SMILE - ) - ) { - return reader.apply(parser); - } - } catch (CorruptIndexException | IndexFormatTooOldException | IndexFormatTooNewException ex) { - // we trick this into a dedicated exception with the original stacktrace - throw new CorruptStateException(ex); - } + return deserialize(blobName, namedXContentRegistry, bytes, XContentType.SMILE, codec, VERSION); } /** @@ -260,4 +227,9 @@ public BytesReference serialize(final T obj, final String blobName, final Compre throws IOException { return serialize(obj, blobName, compressor, params, XContentType.SMILE, codec, VERSION); } + + @Override + CheckedFunction reader() { + return reader; + } } diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java index 8127bf8c2a2a2..48855b18a5981 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java @@ -8,17 +8,21 @@ package org.opensearch.repositories.blobstore; +import org.opensearch.common.CheckedFunction; import org.opensearch.common.blobstore.AsyncMultiStreamBlobContainer; import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.blobstore.stream.write.WritePriority; import org.opensearch.common.blobstore.transfer.RemoteTransferContainer; import org.opensearch.common.blobstore.transfer.stream.OffsetRangeIndexInputStream; +import org.opensearch.common.io.Streams; import org.opensearch.common.lucene.store.ByteArrayIndexInput; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.compress.NoneCompressor; +import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentParser; import java.io.IOException; @@ -35,11 +39,14 @@ */ public class ConfigBlobStoreFormat extends BaseBlobStoreFormat { + private final CheckedFunction reader; + /** * @param blobNameFormat format of the blobname in {@link String#format} format */ - public ConfigBlobStoreFormat(String blobNameFormat) { + public ConfigBlobStoreFormat(String blobNameFormat, CheckedFunction reader) { super(blobNameFormat, true); + this.reader = reader; } public void writeAsyncWithUrgentPriority(T obj, BlobContainer blobContainer, String name, ActionListener listener) @@ -76,4 +83,21 @@ public void writeAsyncWithUrgentPriority(T obj, BlobContainer blobContainer, Str ((AsyncMultiStreamBlobContainer) blobContainer).asyncBlobUpload(remoteTransferContainer.createWriteContext(), listener); } } + + public T read(BlobContainer blobContainer, String name, NamedXContentRegistry namedXContentRegistry) throws IOException { + String blobName = blobName(name); + return deserialize( + blobName, + namedXContentRegistry, + Streams.readFully(blobContainer.readBlob(blobName)), + XContentType.JSON, + null, + null + ); + } + + @Override + CheckedFunction reader() { + return reader; + } } diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java new file mode 100644 index 0000000000000..1b0403886e69a --- /dev/null +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java @@ -0,0 +1,95 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.snapshots; + +import com.fasterxml.jackson.core.JsonParseException; + +import org.opensearch.OpenSearchParseException; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentParser; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * Snapshot Shard path information. + * + * @opensearch.internal + */ +public class SnapshotShardPaths implements ToXContent { + + public static final String DIR = "snapshot_shard_paths"; + + public static final String DELIMITER = "#"; + + public static final String FILE_NAME_FORMAT = "%s"; + + private static final String PATHS_FIELD = "paths"; + + private final List paths; + + public SnapshotShardPaths(List paths) { + this.paths = Collections.unmodifiableList(paths); + } + + public List getPaths() { + return paths; + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startArray(PATHS_FIELD); + for (String path : paths) { + builder.value(path); + } + builder.endArray(); + return builder; + } + + public static SnapshotShardPaths fromXContent(XContentParser parser) throws IOException { + List paths = new ArrayList<>(); + + try { + XContentParser.Token token = parser.currentToken(); + if (token == null) { + token = parser.nextToken(); + } + + if (token != XContentParser.Token.START_OBJECT) { + throw new OpenSearchParseException("Expected a start object"); + } + + token = parser.nextToken(); + if (token == XContentParser.Token.END_OBJECT) { + throw new OpenSearchParseException("Missing [" + PATHS_FIELD + "] field"); + } + + while (token != XContentParser.Token.END_OBJECT) { + String fieldName = parser.currentName(); + if (PATHS_FIELD.equals(fieldName)) { + if (parser.nextToken() != XContentParser.Token.START_ARRAY) { + throw new OpenSearchParseException("Expected an array for field [" + PATHS_FIELD + "]"); + } + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { + paths.add(parser.text()); + } + } else { + throw new OpenSearchParseException("Unexpected field [" + fieldName + "]"); + } + token = parser.nextToken(); + } + } catch (JsonParseException e) { + throw new OpenSearchParseException("Failed to parse SnapshotIndexIdPaths", e); + } + return new SnapshotShardPaths(paths); + } +} diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java index 5e49208465dbb..4e526bdf290a1 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java @@ -136,6 +136,7 @@ import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode; import static org.opensearch.node.remotestore.RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING; import static org.opensearch.repositories.blobstore.BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY; +import static org.opensearch.repositories.blobstore.BlobStoreRepository.SHARD_PATH_TYPE; import static org.opensearch.snapshots.SnapshotUtils.validateSnapshotsBackingAnyIndex; /** @@ -320,9 +321,13 @@ public ClusterState execute(ClusterState currentState) { logger.trace("[{}][{}] creating snapshot for indices [{}]", repositoryName, snapshotName, indices); + int pathType = clusterService.state().nodes().getMinNodeVersion().onOrAfter(Version.CURRENT) + ? SHARD_PATH_TYPE.get(repository.getMetadata().settings()).getCode() + : IndexId.DEFAULT_SHARD_PATH_TYPE; final List indexIds = repositoryData.resolveNewIndices( indices, - getInFlightIndexIds(runningSnapshots, repositoryName) + getInFlightIndexIds(runningSnapshots, repositoryName), + pathType ); final Version version = minCompatibleVersion(currentState.nodes().getMinNodeVersion(), repositoryData, null); final Map shards = shards( diff --git a/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java b/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java index 46293e6a0db7a..9c047c3744c97 100644 --- a/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java +++ b/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java @@ -332,7 +332,7 @@ public void testIndexThatReferenceANullSnapshot() throws IOException { ); assertThat( e.getMessage(), - equalTo("Detected a corrupted repository, " + "index [docs/_id] references an unknown snapshot uuid [null]") + equalTo("Detected a corrupted repository, " + "index [docs/_id/0] references an unknown snapshot uuid [null]") ); } } diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java new file mode 100644 index 0000000000000..3c932319e583b --- /dev/null +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java @@ -0,0 +1,103 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.snapshots; + +import org.opensearch.OpenSearchParseException; +import org.opensearch.common.xcontent.XContentHelper; +import org.opensearch.common.xcontent.XContentType; +import org.opensearch.common.xcontent.json.JsonXContent; +import org.opensearch.core.common.bytes.BytesReference; +import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +public class SnapshotShardPathsTests extends OpenSearchTestCase { + + public void testToXContent() throws IOException { + List paths = Arrays.asList("path1", "path2", "path3"); + SnapshotShardPaths indexIdPaths = new SnapshotShardPaths(paths); + + BytesReference bytes = XContentHelper.toXContent(indexIdPaths, XContentType.JSON, false); + String expectedJson = "{\"paths\":[\"path1\",\"path2\",\"path3\"]}"; + assertEquals(expectedJson, bytes.utf8ToString()); + } + + public void testToXContentEmptyPaths() throws IOException { + SnapshotShardPaths indexIdPaths = new SnapshotShardPaths(Collections.emptyList()); + + BytesReference bytes = XContentHelper.toXContent(indexIdPaths, XContentType.JSON, false); + String expectedJson = "{\"paths\":[]}"; + assertEquals(expectedJson, bytes.utf8ToString()); + } + + public void testFromXContent() throws IOException { + String json = "{\"paths\":[\"path1\",\"path2\",\"path3\"]}"; + try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { + SnapshotShardPaths indexIdPaths = SnapshotShardPaths.fromXContent(parser); + List expectedPaths = Arrays.asList("path1", "path2", "path3"); + assertEquals(expectedPaths, indexIdPaths.getPaths()); + } + } + + public void testFromXContentEmptyPaths() throws IOException { + String json = "{\"paths\":[]}"; + try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { + SnapshotShardPaths indexIdPaths = SnapshotShardPaths.fromXContent(parser); + assertEquals(Collections.emptyList(), indexIdPaths.getPaths()); + } + } + + public void testFromXContentMissingPaths() throws IOException { + String json = "{}"; + try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { + OpenSearchParseException exception = expectThrows( + OpenSearchParseException.class, + () -> SnapshotShardPaths.fromXContent(parser) + ); + assertEquals("Missing [paths] field", exception.getMessage()); + } + } + + public void testFromXContentInvalidJson() throws IOException { + String json = "{\"paths\":\"invalid\"}"; + try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { + OpenSearchParseException exception = expectThrows( + OpenSearchParseException.class, + () -> SnapshotShardPaths.fromXContent(parser) + ); + assertEquals("Expected an array for field [paths]", exception.getMessage()); + } + } + + public void testFromXContentInvalidJsonObject() throws IOException { + String json = "invalid"; + try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { + OpenSearchParseException exception = expectThrows( + OpenSearchParseException.class, + () -> SnapshotShardPaths.fromXContent(parser) + ); + assertEquals("Failed to parse SnapshotIndexIdPaths", exception.getMessage()); + } + } + + public void testFromXContentNotAnObject() throws IOException { + String json = "[]"; + try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { + OpenSearchParseException exception = expectThrows( + OpenSearchParseException.class, + () -> SnapshotShardPaths.fromXContent(parser) + ); + assertEquals("Expected a start object", exception.getMessage()); + } + } +} From d0c0fae42e7f3edd1bde062853de0deb044da3d2 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 28 Aug 2024 01:25:43 +0530 Subject: [PATCH 02/12] Add UTs Signed-off-by: Ashish Singh --- .../index/remote/RemoteIndexPathUploader.java | 3 +- .../index/remote/RemoteStorePathStrategy.java | 4 + .../blobstore/BaseBlobStoreFormat.java | 58 +----- .../blobstore/BlobStoreRepository.java | 86 +++----- .../blobstore/ChecksumBlobStoreFormat.java | 40 +++- .../blobstore/ConfigBlobStoreFormat.java | 26 +-- .../snapshots/SnapshotShardPaths.java | 137 ++++++++----- .../index/remote/RemoteStoreEnumsTests.java | 42 ++++ .../remote/RemoteStorePathStrategyTests.java | 40 ++++ .../opensearch/repositories/IndexIdTests.java | 71 +++++-- .../repositories/RepositoryDataTests.java | 92 ++++++++- .../blobstore/BlobStoreRepositoryTests.java | 190 ++++++++++++++++++ .../snapshots/SnapshotShardPathsTests.java | 163 +++++++++------ 13 files changed, 675 insertions(+), 277 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java b/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java index 5457fe80d6e2d..d5617bdfd94a7 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteIndexPathUploader.java @@ -64,8 +64,7 @@ public class RemoteIndexPathUploader extends IndexMetadataUploadListener { public static final String DELIMITER = "#"; public static final ConfigBlobStoreFormat REMOTE_INDEX_PATH_FORMAT = new ConfigBlobStoreFormat<>( - RemoteIndexPath.FILE_NAME_FORMAT, - RemoteIndexPath::fromXContent + RemoteIndexPath.FILE_NAME_FORMAT ); private static final String TIMEOUT_EXCEPTION_MSG = "Timed out waiting while uploading remote index path file for indexes=%s"; diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java index cd8949288ae3b..7bead3e75f12e 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java @@ -171,6 +171,10 @@ BlobPath hashPath() { return BlobPath.cleanPath().add(shardId).add(indexUUID()); } + public String shardId() { + return shardId; + } + /** * Returns a new builder for {@link SnapshotShardPathInput}. */ diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java index 524effdd41ad8..262a32fa1e74d 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BaseBlobStoreFormat.java @@ -9,34 +9,19 @@ package org.opensearch.repositories.blobstore; import org.apache.lucene.codecs.CodecUtil; -import org.apache.lucene.index.CorruptIndexException; -import org.apache.lucene.index.IndexFormatTooNewException; -import org.apache.lucene.index.IndexFormatTooOldException; -import org.apache.lucene.store.ByteBuffersDataInput; -import org.apache.lucene.store.ByteBuffersIndexInput; -import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.OutputStreamIndexOutput; -import org.apache.lucene.util.BytesRef; -import org.opensearch.common.CheckedFunction; import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.io.stream.BytesStreamOutput; -import org.opensearch.common.lucene.store.ByteArrayIndexInput; import org.opensearch.common.lucene.store.IndexOutputOutputStream; -import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.compress.Compressor; import org.opensearch.core.xcontent.MediaTypeRegistry; -import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.core.xcontent.XContentParser; -import org.opensearch.gateway.CorruptStateException; import java.io.IOException; import java.io.OutputStream; -import java.util.Arrays; import java.util.Locale; import java.util.Objects; @@ -45,7 +30,7 @@ * * @opensearch.internal */ -public abstract class BaseBlobStoreFormat { +public class BaseBlobStoreFormat { private static final int BUFFER_SIZE = 4096; @@ -106,7 +91,7 @@ public BytesReference serialize( try (BytesStreamOutput outputStream = new BytesStreamOutput()) { try ( OutputStreamIndexOutput indexOutput = new OutputStreamIndexOutput( - "BaseBlobStoreFormat.writeBlob(blob=\"" + blobName + "\")", + "ChecksumBlobStoreFormat.writeBlob(blob=\"" + blobName + "\")", blobName, outputStream, BUFFER_SIZE @@ -142,43 +127,4 @@ public void close() { protected String getBlobNameFormat() { return blobNameFormat; } - - public T deserialize( - String blobName, - NamedXContentRegistry namedXContentRegistry, - BytesReference bytes, - XContentType xContentType, - String codec, - Integer version - ) throws IOException { - assert skipHeaderFooter || (Objects.nonNull(codec) && Objects.nonNull(version)); - final String resourceDesc = "BaseBlobStoreFormat.readBlob(blob=\"" + blobName + "\")"; - try { - final IndexInput indexInput = bytes.length() > 0 - ? new ByteBuffersIndexInput(new ByteBuffersDataInput(Arrays.asList(BytesReference.toByteBuffers(bytes))), resourceDesc) - : new ByteArrayIndexInput(resourceDesc, BytesRef.EMPTY_BYTES); - if (skipHeaderFooter == false) { - CodecUtil.checksumEntireFile(indexInput); - CodecUtil.checkHeader(indexInput, codec, version, version); - } - int footerLength = skipHeaderFooter ? 0 : CodecUtil.footerLength(); - long filePointer = indexInput.getFilePointer(); - long contentSize = indexInput.length() - footerLength - filePointer; - try ( - XContentParser parser = XContentHelper.createParser( - namedXContentRegistry, - LoggingDeprecationHandler.INSTANCE, - bytes.slice((int) filePointer, (int) contentSize), - xContentType - ) - ) { - return reader().apply(parser); - } - } catch (CorruptIndexException | IndexFormatTooOldException | IndexFormatTooNewException ex) { - // we trick this into a dedicated exception with the original stacktrace - throw new CorruptStateException(ex); - } - } - - abstract CheckedFunction reader(); } 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 f40f43b8b8d90..7b7ffaa4b682e 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -148,6 +148,7 @@ import org.opensearch.snapshots.SnapshotInfo; import org.opensearch.snapshots.SnapshotMissingException; import org.opensearch.snapshots.SnapshotShardPaths; +import org.opensearch.snapshots.SnapshotShardPaths.ShardInfo; import org.opensearch.snapshots.SnapshotsService; import org.opensearch.threadpool.ThreadPool; @@ -393,8 +394,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp ); public static final ConfigBlobStoreFormat SNAPSHOT_SHARD_PATHS_FORMAT = new ConfigBlobStoreFormat<>( - SnapshotShardPaths.FILE_NAME_FORMAT, - SnapshotShardPaths::fromXContent + SnapshotShardPaths.FILE_NAME_FORMAT ); private volatile boolean readOnly; @@ -1705,7 +1705,7 @@ private List cleanupStaleRootFiles( return Collections.emptyList(); } - private void cleanupStaleIndices( + void cleanupStaleIndices( Map foundIndices, Set survivingIndexIds, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, @@ -1850,10 +1850,10 @@ private void cleanupRemoteStoreLocks( if (highestGenShardPaths.isEmpty()) { releaseRemoteStoreLocksAndCleanup(indexEntry.getValue().children(), remoteStoreLockManagerFactory); } else { - ShardInfo shardInfo = parseShardPath(highestGenShardPaths.get()); - Map shardContainers = new HashMap<>(shardInfo.shardCount); - for (int i = 0; i < shardInfo.shardCount; i++) { - shardContainers.put(String.valueOf(i), shardContainer(shardInfo.indexId, i)); + ShardInfo shardInfo = SnapshotShardPaths.parseShardPath(highestGenShardPaths.get()); + Map shardContainers = new HashMap<>(shardInfo.getShardCount()); + for (int i = 0; i < shardInfo.getShardCount(); i++) { + shardContainers.put(String.valueOf(i), shardContainer(shardInfo.getIndexId(), i)); } releaseRemoteStoreLocksAndCleanup(shardContainers, remoteStoreLockManagerFactory); } @@ -1866,7 +1866,7 @@ private void cleanupRemoteStoreLocks( * @param remoteStoreLockManagerFactory Factory for creating remote store lock managers * @throws IOException If an I/O error occurs during the release and cleanup process */ - private void releaseRemoteStoreLocksAndCleanup( + void releaseRemoteStoreLocksAndCleanup( Map shardBlobs, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory ) throws IOException { @@ -1902,11 +1902,11 @@ private DeleteResult deleteShardData( return DeleteResult.ZERO; } - ShardInfo shardInfo = parseShardPath(highestGenShardPaths.get()); + ShardInfo shardInfo = SnapshotShardPaths.parseShardPath(highestGenShardPaths.get()); DeleteResult deleteResult = DeleteResult.ZERO; - for (int i = 0; i < shardInfo.shardCount; i++) { - deleteResult = deleteResult.add(shardContainer(shardInfo.indexId, i).delete()); + for (int i = 0; i < shardInfo.getShardCount(); i++) { + deleteResult = deleteResult.add(shardContainer(shardInfo.getIndexId(), i).delete()); } deleteFromContainer(snapshotShardPathBlobContainer(), matchingShardPaths); @@ -1914,28 +1914,6 @@ private DeleteResult deleteShardData( return deleteResult.add(matchingShardPaths.size(), totalBytes); } - /** - * Parses a shard path string and extracts relevant shard information. - * - * @param shardPath The shard path string to parse. Expected format is: - * [index_id]#[index_name]#[shard_count]#[path_type_code]#[path_hash_algorithm_code] - * @return A {@link ShardInfo} object containing the parsed index ID and shard count. - * @throws IllegalArgumentException if the shard path format is invalid or cannot be parsed. - */ - private ShardInfo parseShardPath(String shardPath) { - String[] parts = shardPath.split(SnapshotShardPaths.DELIMITER); - if (parts.length != 5) { - throw new IllegalArgumentException("Invalid shard path format: " + shardPath); - } - try { - IndexId indexId = new IndexId(parts[1], parts[0], Integer.parseInt(parts[3])); - int shardCount = Integer.parseInt(parts[2]); - return new ShardInfo(indexId, shardCount); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("Invalid shard path format: " + shardPath, e); - } - } - @Override public void finalizeSnapshot( final ShardGenerations shardGenerations, @@ -2073,20 +2051,27 @@ private Set writeNewIndexShardPaths( return updatedIndexIds; } - private String writeIndexShardPaths(IndexId indexId, SnapshotId snapshotId, int shardCount) { + String writeIndexShardPaths(IndexId indexId, SnapshotId snapshotId, int shardCount) { try { List paths = getShardPaths(indexId, shardCount); - String pathType = String.valueOf(indexId.getShardPathType()); - String pathHashAlgorithm = String.valueOf(FNV_1A_COMPOSITE_1.getCode()); + int pathType = indexId.getShardPathType(); + int pathHashAlgorithm = FNV_1A_COMPOSITE_1.getCode(); String blobName = String.join( SnapshotShardPaths.DELIMITER, indexId.getId(), indexId.getName(), String.valueOf(shardCount), - pathType, - pathHashAlgorithm + String.valueOf(pathType), + String.valueOf(pathHashAlgorithm) + ); + SnapshotShardPaths shardPaths = new SnapshotShardPaths( + paths, + indexId.getId(), + indexId.getName(), + shardCount, + PathType.fromCode(pathType), + PathHashAlgorithm.fromCode(pathHashAlgorithm) ); - SnapshotShardPaths shardPaths = new SnapshotShardPaths(paths); SNAPSHOT_SHARD_PATHS_FORMAT.writeAsyncWithUrgentPriority( shardPaths, snapshotShardPathBlobContainer(), @@ -4003,27 +3988,4 @@ public String toString() { return name; } } - - /** - * Represents parsed information from a shard path. - * This class encapsulates the index ID and shard count extracted from a shard path string. - */ - private static class ShardInfo { - /** The ID of the index associated with this shard. */ - final IndexId indexId; - - /** The total number of shards for this index. */ - final int shardCount; - - /** - * Constructs a new ShardInfo instance. - * - * @param indexId The ID of the index associated with this shard. - * @param shardCount The total number of shards for this index. - */ - ShardInfo(IndexId indexId, int shardCount) { - this.indexId = indexId; - this.shardCount = shardCount; - } - } } diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java index e6f2218086e78..3a49fed4be282 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java @@ -31,7 +31,14 @@ package org.opensearch.repositories.blobstore; +import org.apache.lucene.codecs.CodecUtil; +import org.apache.lucene.index.CorruptIndexException; +import org.apache.lucene.index.IndexFormatTooNewException; +import org.apache.lucene.index.IndexFormatTooOldException; +import org.apache.lucene.store.ByteBuffersDataInput; +import org.apache.lucene.store.ByteBuffersIndexInput; import org.apache.lucene.store.IndexInput; +import org.apache.lucene.util.BytesRef; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.common.CheckedFunction; import org.opensearch.common.blobstore.AsyncMultiStreamBlobContainer; @@ -41,6 +48,8 @@ import org.opensearch.common.blobstore.transfer.stream.OffsetRangeIndexInputStream; import org.opensearch.common.io.Streams; import org.opensearch.common.lucene.store.ByteArrayIndexInput; +import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.bytes.BytesReference; @@ -48,10 +57,12 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.gateway.CorruptStateException; import org.opensearch.index.store.exception.ChecksumCombinationException; import org.opensearch.snapshots.SnapshotInfo; import java.io.IOException; +import java.util.Arrays; import java.util.HashMap; import java.util.Locale; import java.util.Map; @@ -113,7 +124,29 @@ public String blobName(String name) { } public T deserialize(String blobName, NamedXContentRegistry namedXContentRegistry, BytesReference bytes) throws IOException { - return deserialize(blobName, namedXContentRegistry, bytes, XContentType.SMILE, codec, VERSION); + final String resourceDesc = "ChecksumBlobStoreFormat.readBlob(blob=\"" + blobName + "\")"; + try { + final IndexInput indexInput = bytes.length() > 0 + ? new ByteBuffersIndexInput(new ByteBuffersDataInput(Arrays.asList(BytesReference.toByteBuffers(bytes))), resourceDesc) + : new ByteArrayIndexInput(resourceDesc, BytesRef.EMPTY_BYTES); + CodecUtil.checksumEntireFile(indexInput); + CodecUtil.checkHeader(indexInput, codec, VERSION, VERSION); + long filePointer = indexInput.getFilePointer(); + long contentSize = indexInput.length() - CodecUtil.footerLength() - filePointer; + try ( + XContentParser parser = XContentHelper.createParser( + namedXContentRegistry, + LoggingDeprecationHandler.INSTANCE, + bytes.slice((int) filePointer, (int) contentSize), + XContentType.SMILE + ) + ) { + return reader.apply(parser); + } + } catch (CorruptIndexException | IndexFormatTooOldException | IndexFormatTooNewException ex) { + // we trick this into a dedicated exception with the original stacktrace + throw new CorruptStateException(ex); + } } /** @@ -227,9 +260,4 @@ public BytesReference serialize(final T obj, final String blobName, final Compre throws IOException { return serialize(obj, blobName, compressor, params, XContentType.SMILE, codec, VERSION); } - - @Override - CheckedFunction reader() { - return reader; - } } diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java index 48855b18a5981..8127bf8c2a2a2 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java @@ -8,21 +8,17 @@ package org.opensearch.repositories.blobstore; -import org.opensearch.common.CheckedFunction; import org.opensearch.common.blobstore.AsyncMultiStreamBlobContainer; import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.blobstore.stream.write.WritePriority; import org.opensearch.common.blobstore.transfer.RemoteTransferContainer; import org.opensearch.common.blobstore.transfer.stream.OffsetRangeIndexInputStream; -import org.opensearch.common.io.Streams; import org.opensearch.common.lucene.store.ByteArrayIndexInput; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.compress.NoneCompressor; -import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentParser; import java.io.IOException; @@ -39,14 +35,11 @@ */ public class ConfigBlobStoreFormat extends BaseBlobStoreFormat { - private final CheckedFunction reader; - /** * @param blobNameFormat format of the blobname in {@link String#format} format */ - public ConfigBlobStoreFormat(String blobNameFormat, CheckedFunction reader) { + public ConfigBlobStoreFormat(String blobNameFormat) { super(blobNameFormat, true); - this.reader = reader; } public void writeAsyncWithUrgentPriority(T obj, BlobContainer blobContainer, String name, ActionListener listener) @@ -83,21 +76,4 @@ public void writeAsyncWithUrgentPriority(T obj, BlobContainer blobContainer, Str ((AsyncMultiStreamBlobContainer) blobContainer).asyncBlobUpload(remoteTransferContainer.createWriteContext(), listener); } } - - public T read(BlobContainer blobContainer, String name, NamedXContentRegistry namedXContentRegistry) throws IOException { - String blobName = blobName(name); - return deserialize( - blobName, - namedXContentRegistry, - Streams.readFully(blobContainer.readBlob(blobName)), - XContentType.JSON, - null, - null - ); - } - - @Override - CheckedFunction reader() { - return reader; - } } diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java index 1b0403886e69a..8fa12b028e0f1 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java @@ -8,16 +8,14 @@ package org.opensearch.snapshots; -import com.fasterxml.jackson.core.JsonParseException; - -import org.opensearch.OpenSearchParseException; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; +import org.opensearch.repositories.IndexId; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; import java.util.List; /** @@ -34,19 +32,49 @@ public class SnapshotShardPaths implements ToXContent { public static final String FILE_NAME_FORMAT = "%s"; private static final String PATHS_FIELD = "paths"; + private static final String INDEX_ID_FIELD = "indexId"; + private static final String INDEX_NAME_FIELD = "indexName"; + private static final String NUMBER_OF_SHARDS_FIELD = "number_of_shards"; + private static final String SHARD_PATH_TYPE_FIELD = "shard_path_type"; + private static final String SHARD_PATH_HASH_ALGORITHM_FIELD = "shard_path_hash_algorithm"; private final List paths; - - public SnapshotShardPaths(List paths) { - this.paths = Collections.unmodifiableList(paths); - } - - public List getPaths() { - return paths; + private final String indexId; + private final String indexName; + private final int numberOfShards; + private final PathType shardPathType; + private final PathHashAlgorithm shardPathHashAlgorithm; + + public SnapshotShardPaths( + List paths, + String indexId, + String indexName, + int numberOfShards, + PathType shardPathType, + PathHashAlgorithm shardPathHashAlgorithm + ) { + assert !paths.isEmpty() : "paths must not be empty"; + assert indexId != null && !indexId.isEmpty() : "indexId must not be empty"; + assert indexName != null && !indexName.isEmpty() : "indexName must not be empty"; + assert numberOfShards > 0 : "numberOfShards must be > 0"; + assert shardPathType != null : "shardPathType must not be null"; + assert shardPathHashAlgorithm != null : "shardPathHashAlgorithm must not be null"; + + this.paths = paths; + this.indexId = indexId; + this.indexName = indexName; + this.numberOfShards = numberOfShards; + this.shardPathType = shardPathType; + this.shardPathHashAlgorithm = shardPathHashAlgorithm; } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.field(INDEX_ID_FIELD, indexId); + builder.field(INDEX_NAME_FIELD, indexName); + builder.field(NUMBER_OF_SHARDS_FIELD, numberOfShards); + builder.field(SHARD_PATH_TYPE_FIELD, shardPathType.getCode()); + builder.field(SHARD_PATH_HASH_ALGORITHM_FIELD, shardPathHashAlgorithm.getCode()); builder.startArray(PATHS_FIELD); for (String path : paths) { builder.value(path); @@ -55,41 +83,60 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } - public static SnapshotShardPaths fromXContent(XContentParser parser) throws IOException { - List paths = new ArrayList<>(); + public static SnapshotShardPaths fromXContent(XContentParser ignored) { + throw new UnsupportedOperationException("SnapshotShardPaths.fromXContent() is not supported"); + } + /** + * Parses a shard path string and extracts relevant shard information. + * + * @param shardPath The shard path string to parse. Expected format is: + * [index_id]#[index_name]#[shard_count]#[path_type_code]#[path_hash_algorithm_code] + * @return A {@link ShardInfo} object containing the parsed index ID and shard count. + * @throws IllegalArgumentException if the shard path format is invalid or cannot be parsed. + */ + public static ShardInfo parseShardPath(String shardPath) { + String[] parts = shardPath.split(SnapshotShardPaths.DELIMITER); + if (parts.length != 5) { + throw new IllegalArgumentException("Invalid shard path format: " + shardPath); + } try { - XContentParser.Token token = parser.currentToken(); - if (token == null) { - token = parser.nextToken(); - } - - if (token != XContentParser.Token.START_OBJECT) { - throw new OpenSearchParseException("Expected a start object"); - } - - token = parser.nextToken(); - if (token == XContentParser.Token.END_OBJECT) { - throw new OpenSearchParseException("Missing [" + PATHS_FIELD + "] field"); - } - - while (token != XContentParser.Token.END_OBJECT) { - String fieldName = parser.currentName(); - if (PATHS_FIELD.equals(fieldName)) { - if (parser.nextToken() != XContentParser.Token.START_ARRAY) { - throw new OpenSearchParseException("Expected an array for field [" + PATHS_FIELD + "]"); - } - while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - paths.add(parser.text()); - } - } else { - throw new OpenSearchParseException("Unexpected field [" + fieldName + "]"); - } - token = parser.nextToken(); - } - } catch (JsonParseException e) { - throw new OpenSearchParseException("Failed to parse SnapshotIndexIdPaths", e); + IndexId indexId = new IndexId(parts[1], parts[0], Integer.parseInt(parts[3])); + int shardCount = Integer.parseInt(parts[2]); + return new ShardInfo(indexId, shardCount); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid shard path format: " + shardPath, e); + } + } + + /** + * Represents parsed information from a shard path. + * This class encapsulates the index ID and shard count extracted from a shard path string. + */ + public static class ShardInfo { + /** The ID of the index associated with this shard. */ + private final IndexId indexId; + + /** The total number of shards for this index. */ + private final int shardCount; + + /** + * Constructs a new ShardInfo instance. + * + * @param indexId The ID of the index associated with this shard. + * @param shardCount The total number of shards for this index. + */ + ShardInfo(IndexId indexId, int shardCount) { + this.indexId = indexId; + this.shardCount = shardCount; + } + + public IndexId getIndexId() { + return indexId; + } + + public int getShardCount() { + return shardCount; } - return new SnapshotShardPaths(paths); } } diff --git a/server/src/test/java/org/opensearch/index/remote/RemoteStoreEnumsTests.java b/server/src/test/java/org/opensearch/index/remote/RemoteStoreEnumsTests.java index 481a0568ff0a7..cfcac5915837c 100644 --- a/server/src/test/java/org/opensearch/index/remote/RemoteStoreEnumsTests.java +++ b/server/src/test/java/org/opensearch/index/remote/RemoteStoreEnumsTests.java @@ -14,6 +14,7 @@ import org.opensearch.index.remote.RemoteStoreEnums.DataType; import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.index.remote.RemoteStorePathStrategy.ShardDataPathInput; +import org.opensearch.index.remote.RemoteStorePathStrategy.SnapshotShardPathInput; import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; @@ -597,6 +598,47 @@ public void testGeneratePathForHashedInfixType() { assertTrue(new ParameterizedMessage("expected={} actual={}", expected, actual).getFormattedMessage(), actual.startsWith(expected)); } + public void testGeneratePathForSnapshotShardPathInput() { + BlobPath blobPath = BlobPath.cleanPath().add("xjsdhj").add("ddjsha").add("yudy7sd").add("32hdhua7").add("89jdij"); + String indexUUID = "dsdkjsu8832njn"; + String shardId = "10"; + SnapshotShardPathInput pathInput = SnapshotShardPathInput.builder() + .basePath(blobPath) + .indexUUID(indexUUID) + .shardId(shardId) + .build(); + + // FIXED PATH + BlobPath result = FIXED.path(pathInput, null); + String expected = "xjsdhj/ddjsha/yudy7sd/32hdhua7/89jdij/indices/dsdkjsu8832njn/10/"; + String actual = result.buildAsString(); + assertEquals(new ParameterizedMessage("expected={} actual={}", expected, actual).getFormattedMessage(), actual, expected); + + // HASHED_PREFIX - FNV_1A_COMPOSITE_1 + result = HASHED_PREFIX.path(pathInput, FNV_1A_COMPOSITE_1); + expected = "_11001000010110/xjsdhj/ddjsha/yudy7sd/32hdhua7/89jdij/indices/dsdkjsu8832njn/10/"; + actual = result.buildAsString(); + assertEquals(new ParameterizedMessage("expected={} actual={}", expected, actual).getFormattedMessage(), actual, expected); + + // HASHED_PREFIX - FNV_1A_BASE64 + result = HASHED_PREFIX.path(pathInput, FNV_1A_BASE64); + expected = "_yFiSl_VGGM/xjsdhj/ddjsha/yudy7sd/32hdhua7/89jdij/indices/dsdkjsu8832njn/10/"; + actual = result.buildAsString(); + assertEquals(new ParameterizedMessage("expected={} actual={}", expected, actual).getFormattedMessage(), actual, expected); + + // HASHED_INFIX - FNV_1A_COMPOSITE_1 + result = HASHED_INFIX.path(pathInput, FNV_1A_COMPOSITE_1); + expected = "xjsdhj/ddjsha/yudy7sd/32hdhua7/89jdij/_11001000010110/indices/dsdkjsu8832njn/10/"; + actual = result.buildAsString(); + assertEquals(new ParameterizedMessage("expected={} actual={}", expected, actual).getFormattedMessage(), actual, expected); + + // HASHED_INFIX - FNV_1A_BASE64 + result = HASHED_INFIX.path(pathInput, FNV_1A_BASE64); + expected = "xjsdhj/ddjsha/yudy7sd/32hdhua7/89jdij/_yFiSl_VGGM/indices/dsdkjsu8832njn/10/"; + actual = result.buildAsString(); + assertEquals(new ParameterizedMessage("expected={} actual={}", expected, actual).getFormattedMessage(), actual, expected); + } + private String derivePath(String basePath, ShardDataPathInput pathInput) { return "".equals(basePath) ? String.join( diff --git a/server/src/test/java/org/opensearch/index/remote/RemoteStorePathStrategyTests.java b/server/src/test/java/org/opensearch/index/remote/RemoteStorePathStrategyTests.java index cf5876cb5caf1..c27895e4e96b5 100644 --- a/server/src/test/java/org/opensearch/index/remote/RemoteStorePathStrategyTests.java +++ b/server/src/test/java/org/opensearch/index/remote/RemoteStorePathStrategyTests.java @@ -82,6 +82,46 @@ public void testFixedSubPath() { .dataType(DATA) .build(); assertEquals(BlobPath.cleanPath().add(INDEX_UUID).add(SHARD_ID).add(TRANSLOG.getName()).add(DATA.getName()), input2.fixedSubPath()); + } + + public void testSnapshotShardPathInput() { + assertThrows(NullPointerException.class, () -> RemoteStorePathStrategy.SnapshotShardPathInput.builder().build()); + assertThrows( + NullPointerException.class, + () -> RemoteStorePathStrategy.SnapshotShardPathInput.builder().basePath(BASE_PATH).build() + ); + assertThrows( + NullPointerException.class, + () -> RemoteStorePathStrategy.SnapshotShardPathInput.builder().indexUUID(INDEX_UUID).build() + ); + assertThrows(NullPointerException.class, () -> RemoteStorePathStrategy.SnapshotShardPathInput.builder().shardId(SHARD_ID).build()); + + RemoteStorePathStrategy.SnapshotShardPathInput input = RemoteStorePathStrategy.SnapshotShardPathInput.builder() + .basePath(BASE_PATH) + .indexUUID(INDEX_UUID) + .shardId(SHARD_ID) + .build(); + assertEquals(BASE_PATH, input.basePath()); + assertEquals(INDEX_UUID, input.indexUUID()); + assertEquals(SHARD_ID, input.shardId()); + } + + public void testSnapshotShardPathInputFixedSubPath() { + RemoteStorePathStrategy.SnapshotShardPathInput input = RemoteStorePathStrategy.SnapshotShardPathInput.builder() + .basePath(BASE_PATH) + .indexUUID(INDEX_UUID) + .shardId(SHARD_ID) + .build(); + assertEquals(BlobPath.cleanPath().add("indices").add(INDEX_UUID).add(SHARD_ID), input.fixedSubPath()); + } + public void testSnapshotShardPathInputHashPath() { + RemoteStorePathStrategy.SnapshotShardPathInput input = RemoteStorePathStrategy.SnapshotShardPathInput.builder() + .basePath(BASE_PATH) + .indexUUID(INDEX_UUID) + .shardId(SHARD_ID) + .build(); + assertEquals(BlobPath.cleanPath().add(SHARD_ID).add(INDEX_UUID), input.hashPath()); } + } diff --git a/server/src/test/java/org/opensearch/repositories/IndexIdTests.java b/server/src/test/java/org/opensearch/repositories/IndexIdTests.java index 2b927b3b40115..3b719d287aa9b 100644 --- a/server/src/test/java/org/opensearch/repositories/IndexIdTests.java +++ b/server/src/test/java/org/opensearch/repositories/IndexIdTests.java @@ -32,6 +32,7 @@ package org.opensearch.repositories; +import org.opensearch.Version; import org.opensearch.common.UUIDs; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.xcontent.json.JsonXContent; @@ -39,6 +40,7 @@ import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.index.remote.RemoteStoreEnums; import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; @@ -52,51 +54,84 @@ public void testEqualsAndHashCode() { // assert equals and hashcode String name = randomAlphaOfLength(8); String id = UUIDs.randomBase64UUID(); - IndexId indexId1 = new IndexId(name, id); - IndexId indexId2 = new IndexId(name, id); + int shardPathType = randomIntBetween(0, 2); + IndexId indexId1 = new IndexId(name, id, shardPathType); + IndexId indexId2 = new IndexId(name, id, shardPathType); assertEquals(indexId1, indexId2); assertEquals(indexId1.hashCode(), indexId2.hashCode()); // assert equals when using index name for id id = name; - indexId1 = new IndexId(name, id); - indexId2 = new IndexId(name, id); + indexId1 = new IndexId(name, id, shardPathType); + indexId2 = new IndexId(name, id, shardPathType); assertEquals(indexId1, indexId2); assertEquals(indexId1.hashCode(), indexId2.hashCode()); - // assert not equals when name or id differ - indexId2 = new IndexId(randomAlphaOfLength(8), id); + // assert not equals when name, id, or shardPathType differ + indexId2 = new IndexId(randomAlphaOfLength(8), id, shardPathType); assertNotEquals(indexId1, indexId2); assertNotEquals(indexId1.hashCode(), indexId2.hashCode()); - indexId2 = new IndexId(name, UUIDs.randomBase64UUID()); + indexId2 = new IndexId(name, UUIDs.randomBase64UUID(), shardPathType); assertNotEquals(indexId1, indexId2); assertNotEquals(indexId1.hashCode(), indexId2.hashCode()); + int newShardPathType = randomIntBetween(0, 2); + indexId2 = new IndexId(name, id, newShardPathType); + if (shardPathType == newShardPathType) { + assertEquals(indexId1, indexId2); + assertEquals(indexId1.hashCode(), indexId2.hashCode()); + } else { + assertNotEquals(indexId1, indexId2); + assertNotEquals(indexId1.hashCode(), indexId2.hashCode()); + } } public void testSerialization() throws IOException { - IndexId indexId = new IndexId(randomAlphaOfLength(8), UUIDs.randomBase64UUID()); + IndexId indexId = new IndexId(randomAlphaOfLength(8), UUIDs.randomBase64UUID(), randomIntBetween(0, 2)); BytesStreamOutput out = new BytesStreamOutput(); + out.setVersion(Version.CURRENT); indexId.writeTo(out); assertEquals(indexId, new IndexId(out.bytes().streamInput())); } public void testXContent() throws IOException { - IndexId indexId = new IndexId(randomAlphaOfLength(8), UUIDs.randomBase64UUID()); + String name = randomAlphaOfLength(8); + String id = UUIDs.randomBase64UUID(); + int shardPathType = randomIntBetween(0, 2); + IndexId indexId = new IndexId(name, id, shardPathType); XContentBuilder builder = JsonXContent.contentBuilder(); indexId.toXContent(builder, ToXContent.EMPTY_PARAMS); XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder)); assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); - String name = null; - String id = null; + String parsedName = null; + String parsedId = null; + int parsedShardPathType = -1; while (parser.nextToken() != XContentParser.Token.END_OBJECT) { final String currentFieldName = parser.currentName(); parser.nextToken(); - if (currentFieldName.equals(IndexId.NAME)) { - name = parser.text(); - } else if (currentFieldName.equals(IndexId.ID)) { - id = parser.text(); + switch (currentFieldName) { + case IndexId.NAME: + parsedName = parser.text(); + break; + case IndexId.ID: + parsedId = parser.text(); + break; + case IndexId.SHARD_PATH_TYPE: + parsedShardPathType = parser.intValue(); + break; } } - assertNotNull(name); - assertNotNull(id); - assertEquals(indexId, new IndexId(name, id)); + parser.close(); + assertNotNull(parsedName); + assertNotNull(parsedId); + assertNotEquals(-1, parsedShardPathType); + assertEquals(name, parsedName); + assertEquals(id, parsedId); + assertEquals(shardPathType, parsedShardPathType); + } + + public void testDefaultShardPathType() { + String name = randomAlphaOfLength(8); + String id = UUIDs.randomBase64UUID(); + IndexId indexId = new IndexId(name, id); + assertEquals(IndexId.DEFAULT_SHARD_PATH_TYPE, indexId.getShardPathType()); + assertEquals(RemoteStoreEnums.PathType.FIXED.getCode(), IndexId.DEFAULT_SHARD_PATH_TYPE); } } diff --git a/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java b/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java index 9c047c3744c97..285a1131326ee 100644 --- a/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java +++ b/server/src/test/java/org/opensearch/repositories/RepositoryDataTests.java @@ -42,12 +42,14 @@ import org.opensearch.core.xcontent.XContent; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.snapshots.SnapshotId; import org.opensearch.snapshots.SnapshotState; import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -59,6 +61,8 @@ import java.util.function.Function; import java.util.stream.Collectors; +import static org.opensearch.index.remote.RemoteStoreEnums.PathType.FIXED; +import static org.opensearch.index.remote.RemoteStoreEnums.PathType.HASHED_PREFIX; import static org.opensearch.repositories.RepositoryData.EMPTY_REPO_GEN; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; @@ -70,7 +74,7 @@ public class RepositoryDataTests extends OpenSearchTestCase { public void testEqualsAndHashCode() { - RepositoryData repositoryData1 = generateRandomRepoData(); + RepositoryData repositoryData1 = generateRandomRepoData(FIXED.getCode()); RepositoryData repositoryData2 = repositoryData1.copy(); assertEquals(repositoryData1, repositoryData2); assertEquals(repositoryData1.hashCode(), repositoryData2.hashCode()); @@ -403,11 +407,95 @@ public void testIndexMetaDataToRemoveAfterRemovingSnapshotWithSharing() { assertEquals(newRepoData.indexMetaDataToRemoveAfterRemovingSnapshots(Collections.singleton(otherSnapshotId)), removeFromOther); } + public void testResolveNewIndices() { + // Test case 1: All indices are new + List indicesToResolve = Arrays.asList("index1", "index2", "index3"); + Map inFlightIds = Collections.emptyMap(); + int pathType = randomIntBetween(0, 2); + List resolvedIndices = RepositoryData.EMPTY.resolveNewIndices(indicesToResolve, inFlightIds, pathType); + assertEquals(indicesToResolve.size(), resolvedIndices.size()); + for (IndexId indexId : resolvedIndices) { + assertTrue(indicesToResolve.contains(indexId.getName())); + assertNotNull(indexId.getId()); + assertEquals(pathType, indexId.getShardPathType()); + } + + // Test case 2: Some indices are existing, some are new + RepositoryData repositoryData = generateRandomRepoData(); + Map existingIndices = repositoryData.getIndices(); + List existingIndexNames = new ArrayList<>(existingIndices.keySet()); + List newIndexNames = Arrays.asList("newIndex1", "newIndex2"); + indicesToResolve = new ArrayList<>(existingIndexNames); + indicesToResolve.addAll(newIndexNames); + pathType = randomIntBetween(0, 2); + resolvedIndices = repositoryData.resolveNewIndices(indicesToResolve, Collections.emptyMap(), pathType); + assertEquals(indicesToResolve.size(), resolvedIndices.size()); + for (IndexId indexId : resolvedIndices) { + if (existingIndexNames.contains(indexId.getName())) { + assertEquals(existingIndices.get(indexId.getName()), indexId); + } else { + assertTrue(newIndexNames.contains(indexId.getName())); + assertNotNull(indexId.getId()); + assertEquals(pathType, indexId.getShardPathType()); + } + } + + // Test case 3: Some indices are in-flight + Map inFlightIndexIds = new HashMap<>(); + for (String indexName : newIndexNames) { + inFlightIndexIds.put(indexName, new IndexId(indexName, UUIDs.randomBase64UUID(), pathType)); + } + resolvedIndices = repositoryData.resolveNewIndices(indicesToResolve, inFlightIndexIds, pathType); + assertEquals(indicesToResolve.size(), resolvedIndices.size()); + for (IndexId indexId : resolvedIndices) { + if (existingIndexNames.contains(indexId.getName())) { + assertEquals(existingIndices.get(indexId.getName()), indexId); + } else if (newIndexNames.contains(indexId.getName())) { + assertEquals(inFlightIndexIds.get(indexId.getName()), indexId); + } else { + fail("Unexpected index: " + indexId.getName()); + } + } + } + + public void testResolveNewIndicesWithDifferentPathType() { + // Generate repository data with a fixed path type + int existingPathType = PathType.FIXED.getCode(); + RepositoryData repositoryData = generateRandomRepoData(existingPathType); + Map existingIndices = repositoryData.getIndices(); + + // Create a list of existing and new index names + List existingIndexNames = new ArrayList<>(existingIndices.keySet()); + List newIndexNames = Arrays.asList("newIndex1", "newIndex2"); + List indicesToResolve = new ArrayList<>(existingIndexNames); + indicesToResolve.addAll(newIndexNames); + + // Use a different path type for new indices + int newPathType = HASHED_PREFIX.getCode(); + + List resolvedIndices = repositoryData.resolveNewIndices(indicesToResolve, Collections.emptyMap(), newPathType); + assertEquals(indicesToResolve.size(), resolvedIndices.size()); + for (IndexId indexId : resolvedIndices) { + if (existingIndexNames.contains(indexId.getName())) { + assertEquals(existingIndices.get(indexId.getName()), indexId); + assertEquals(existingPathType, indexId.getShardPathType()); + } else { + assertTrue(newIndexNames.contains(indexId.getName())); + assertNotNull(indexId.getId()); + assertEquals(newPathType, indexId.getShardPathType()); + } + } + } + public static RepositoryData generateRandomRepoData() { + return generateRandomRepoData(randomFrom(PathType.values()).getCode()); + } + + public static RepositoryData generateRandomRepoData(int pathType) { final int numIndices = randomIntBetween(1, 30); final List indices = new ArrayList<>(numIndices); for (int i = 0; i < numIndices; i++) { - indices.add(new IndexId(randomAlphaOfLength(8), UUIDs.randomBase64UUID())); + indices.add(new IndexId(randomAlphaOfLength(8), UUIDs.randomBase64UUID(), pathType)); } final int numSnapshots = randomIntBetween(1, 30); RepositoryData repositoryData = RepositoryData.EMPTY; diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java index bd47507da4863..79204584ad0ce 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -34,18 +34,26 @@ import org.opensearch.Version; import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; +import org.opensearch.action.support.GroupedActionListener; import org.opensearch.action.support.PlainActionFuture; import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.RepositoryMetadata; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.UUIDs; +import org.opensearch.common.blobstore.BlobContainer; +import org.opensearch.common.blobstore.BlobMetadata; +import org.opensearch.common.blobstore.DeleteResult; import org.opensearch.common.settings.Settings; +import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.unit.ByteSizeUnit; import org.opensearch.core.index.Index; import org.opensearch.core.index.shard.ShardId; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; +import org.opensearch.index.remote.RemoteStoreEnums; +import org.opensearch.index.store.lockmanager.RemoteStoreLockManager; +import org.opensearch.index.store.lockmanager.RemoteStoreLockManagerFactory; import org.opensearch.indices.recovery.RecoverySettings; import org.opensearch.plugins.Plugin; import org.opensearch.plugins.RepositoryPlugin; @@ -57,6 +65,8 @@ import org.opensearch.repositories.ShardGenerations; import org.opensearch.repositories.fs.FsRepository; import org.opensearch.snapshots.SnapshotId; +import org.opensearch.snapshots.SnapshotShardPaths; +import org.opensearch.snapshots.SnapshotShardPaths.ShardInfo; import org.opensearch.snapshots.SnapshotState; import org.opensearch.test.OpenSearchIntegTestCase; @@ -64,15 +74,29 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.stream.Collectors; import static org.opensearch.repositories.RepositoryDataTests.generateRandomRepoData; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** * Tests for the {@link BlobStoreRepository} and its subclasses. @@ -370,4 +394,170 @@ public void testRemoteStoreShardCleanupTask() { remoteStoreShardCleanupTask.run(); assertFalse(executed1.get()); } + + public void testParseShardPath() { + RepositoryData repoData = generateRandomRepoData(); + IndexId indexId = repoData.getIndices().values().iterator().next(); + int shardCount = repoData.shardGenerations().getGens(indexId).size(); + + String shardPath = String.join( + SnapshotShardPaths.DELIMITER, + indexId.getId(), + indexId.getName(), + String.valueOf(shardCount), + String.valueOf(indexId.getShardPathType()), + "1" + ); + ShardInfo shardInfo = SnapshotShardPaths.parseShardPath(shardPath); + + assertEquals(shardInfo.getIndexId(), indexId); + assertEquals(shardInfo.getShardCount(), shardCount); + } + + public void testWriteAndReadShardPaths() throws Exception { + BlobStoreRepository repository = setupRepo(); + RepositoryData repoData = generateRandomRepoData(); + SnapshotId snapshotId = repoData.getSnapshotIds().iterator().next(); + + Set writtenShardPaths = new HashSet<>(); + for (IndexId indexId : repoData.getIndices().values()) { + if (indexId.getShardPathType() != IndexId.DEFAULT_SHARD_PATH_TYPE) { + String shardPathBlobName = repository.writeIndexShardPaths(indexId, snapshotId, indexId.getShardPathType()); + writtenShardPaths.add(shardPathBlobName); + } + } + + // Read shard paths and verify + Map shardPathBlobs = repository.snapshotShardPathBlobContainer().listBlobs(); + + // Create sets for comparison + Set expectedPaths = new HashSet<>(writtenShardPaths); + Set actualPaths = new HashSet<>(shardPathBlobs.keySet()); + + // Remove known extra files - "extra0" file is added by the ExtrasFS, which is part of Lucene's test framework + actualPaths.remove("extra0"); + + // Check if all expected paths are present in the actual paths + assertTrue("All expected paths should be present", actualPaths.containsAll(expectedPaths)); + + // Check if there are any unexpected additional paths + Set unexpectedPaths = new HashSet<>(actualPaths); + unexpectedPaths.removeAll(expectedPaths); + if (!unexpectedPaths.isEmpty()) { + logger.warn("Unexpected additional paths found: " + unexpectedPaths); + } + + assertEquals("Expected and actual paths should match after removing known extra files", expectedPaths, actualPaths); + + for (String shardPathBlobName : expectedPaths) { + SnapshotShardPaths.ShardInfo shardInfo = SnapshotShardPaths.parseShardPath(shardPathBlobName); + IndexId indexId = repoData.getIndices().get(shardInfo.getIndexId().getName()); + assertNotNull("IndexId should not be null", indexId); + assertEquals("Index ID should match", shardInfo.getIndexId().getId(), indexId.getId()); + assertEquals("Shard path type should match", shardInfo.getIndexId().getShardPathType(), indexId.getShardPathType()); + String[] parts = shardPathBlobName.split(SnapshotShardPaths.DELIMITER); + assertEquals( + "Path hash algorithm should be FNV_1A_COMPOSITE_1", + RemoteStoreEnums.PathHashAlgorithm.FNV_1A_COMPOSITE_1, + RemoteStoreEnums.PathHashAlgorithm.fromCode(Integer.parseInt(parts[4])) + ); + } + } + + public void testCleanupStaleIndices() throws Exception { + // Mock the BlobStoreRepository + BlobStoreRepository repository = mock(BlobStoreRepository.class); + + // Mock BlobContainer for stale index + BlobContainer staleIndexContainer = mock(BlobContainer.class); + when(staleIndexContainer.delete()).thenReturn(new DeleteResult(1, 100L)); + + // Mock BlobContainer for current index + BlobContainer currentIndexContainer = mock(BlobContainer.class); + + Map foundIndices = new HashMap<>(); + foundIndices.put("stale-index", staleIndexContainer); + foundIndices.put("current-index", currentIndexContainer); + + Set survivingIndexIds = new HashSet<>(); + survivingIndexIds.add("current-index"); + + // Create a mock RemoteStoreLockManagerFactory + RemoteStoreLockManagerFactory mockRemoteStoreLockManagerFactory = mock(RemoteStoreLockManagerFactory.class); + RemoteStoreLockManager mockLockManager = mock(RemoteStoreLockManager.class); + when(mockRemoteStoreLockManagerFactory.newLockManager(anyString(), anyString(), anyString(), any())).thenReturn(mockLockManager); + + // Create mock snapshot shard paths + Map mockSnapshotShardPaths = new HashMap<>(); + String validShardPath = "stale-index-id#stale-index#1#0#1"; + mockSnapshotShardPaths.put(validShardPath, mock(BlobMetadata.class)); + + // Mock snapshotShardPathBlobContainer + BlobContainer mockSnapshotShardPathBlobContainer = mock(BlobContainer.class); + when(mockSnapshotShardPathBlobContainer.delete()).thenReturn(new DeleteResult(1, 50L)); + when(repository.snapshotShardPathBlobContainer()).thenReturn(mockSnapshotShardPathBlobContainer); + + // Mock the cleanupStaleIndices method to call our test implementation + doAnswer(invocation -> { + Map indices = invocation.getArgument(0); + Set surviving = invocation.getArgument(1); + GroupedActionListener listener = invocation.getArgument(3); + + // Simulate the cleanup process + DeleteResult result = DeleteResult.ZERO; + for (Map.Entry entry : indices.entrySet()) { + if (!surviving.contains(entry.getKey())) { + result = result.add(entry.getValue().delete()); + } + } + result = result.add(mockSnapshotShardPathBlobContainer.delete()); + + listener.onResponse(result); + return null; + }).when(repository).cleanupStaleIndices(any(), any(), any(), any(), any()); + + AtomicReference> resultReference = new AtomicReference<>(); + CountDownLatch latch = new CountDownLatch(1); + + GroupedActionListener listener = new GroupedActionListener<>(ActionListener.wrap(deleteResults -> { + resultReference.set(deleteResults); + latch.countDown(); + }, e -> { + logger.error("Error in cleanupStaleIndices", e); + latch.countDown(); + }), 1); + + // Call the method we're testing + repository.cleanupStaleIndices( + foundIndices, + survivingIndexIds, + mockRemoteStoreLockManagerFactory, + listener, + mockSnapshotShardPaths + ); + + assertTrue("Cleanup did not complete within the expected time", latch.await(30, TimeUnit.SECONDS)); + + Collection results = resultReference.get(); + assertNotNull("DeleteResult collection should not be null", results); + assertFalse("DeleteResult collection should not be empty", results.isEmpty()); + + DeleteResult combinedResult = results.stream().reduce(DeleteResult.ZERO, DeleteResult::add); + + assertTrue("Bytes deleted should be greater than 0", combinedResult.bytesDeleted() > 0); + assertTrue("Blobs deleted should be greater than 0", combinedResult.blobsDeleted() > 0); + + // Verify that the stale index was processed for deletion + verify(staleIndexContainer, times(1)).delete(); + + // Verify that the current index was not processed for deletion + verify(currentIndexContainer, never()).delete(); + + // Verify that snapshot shard paths were considered in the cleanup process + verify(mockSnapshotShardPathBlobContainer, times(1)).delete(); + + // Verify the total number of bytes and blobs deleted + assertEquals("Total bytes deleted should be 150", 150L, combinedResult.bytesDeleted()); + assertEquals("Total blobs deleted should be 2", 2, combinedResult.blobsDeleted()); + } } diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java index 3c932319e583b..15eb70913eb88 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotShardPathsTests.java @@ -8,12 +8,10 @@ package org.opensearch.snapshots; -import org.opensearch.OpenSearchParseException; import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.common.xcontent.json.JsonXContent; import org.opensearch.core.common.bytes.BytesReference; -import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.index.remote.RemoteStoreEnums; import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; @@ -24,80 +22,123 @@ public class SnapshotShardPathsTests extends OpenSearchTestCase { public void testToXContent() throws IOException { - List paths = Arrays.asList("path1", "path2", "path3"); - SnapshotShardPaths indexIdPaths = new SnapshotShardPaths(paths); + List paths = Arrays.asList("/path/to/shard/1", "/path/to/shard/2", "/path/to/shard/3"); + String indexId = "index-id"; + String indexName = "index-name"; + int numberOfShards = 5; + RemoteStoreEnums.PathType shardPathType = RemoteStoreEnums.PathType.HASHED_PREFIX; + RemoteStoreEnums.PathHashAlgorithm shardPathHashAlgorithm = RemoteStoreEnums.PathHashAlgorithm.FNV_1A_BASE64; - BytesReference bytes = XContentHelper.toXContent(indexIdPaths, XContentType.JSON, false); - String expectedJson = "{\"paths\":[\"path1\",\"path2\",\"path3\"]}"; + SnapshotShardPaths snapshotShardPaths = new SnapshotShardPaths( + paths, + indexId, + indexName, + numberOfShards, + shardPathType, + shardPathHashAlgorithm + ); + + BytesReference bytes = XContentHelper.toXContent(snapshotShardPaths, XContentType.JSON, false); + String expectedJson = + "{\"indexId\":\"index-id\",\"indexName\":\"index-name\",\"number_of_shards\":5,\"shard_path_type\":1,\"shard_path_hash_algorithm\":0,\"paths\":[\"/path/to/shard/1\",\"/path/to/shard/2\",\"/path/to/shard/3\"]}"; assertEquals(expectedJson, bytes.utf8ToString()); } - public void testToXContentEmptyPaths() throws IOException { - SnapshotShardPaths indexIdPaths = new SnapshotShardPaths(Collections.emptyList()); + public void testMissingPaths() { + List paths = Collections.emptyList(); + String indexId = "index-id"; + String indexName = "index-name"; + int numberOfShards = 5; + RemoteStoreEnums.PathType shardPathType = RemoteStoreEnums.PathType.FIXED; + RemoteStoreEnums.PathHashAlgorithm shardPathHashAlgorithm = RemoteStoreEnums.PathHashAlgorithm.FNV_1A_COMPOSITE_1; - BytesReference bytes = XContentHelper.toXContent(indexIdPaths, XContentType.JSON, false); - String expectedJson = "{\"paths\":[]}"; - assertEquals(expectedJson, bytes.utf8ToString()); + AssertionError exception = expectThrows( + AssertionError.class, + () -> new SnapshotShardPaths(paths, indexId, indexName, numberOfShards, shardPathType, shardPathHashAlgorithm) + ); + assertTrue(exception.getMessage().contains("paths must not be empty")); } - public void testFromXContent() throws IOException { - String json = "{\"paths\":[\"path1\",\"path2\",\"path3\"]}"; - try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { - SnapshotShardPaths indexIdPaths = SnapshotShardPaths.fromXContent(parser); - List expectedPaths = Arrays.asList("path1", "path2", "path3"); - assertEquals(expectedPaths, indexIdPaths.getPaths()); - } + public void testMissingIndexId() { + List paths = Arrays.asList("/path/to/shard/1", "/path/to/shard/2", "/path/to/shard/3"); + String indexId = ""; + String indexName = "index-name"; + int numberOfShards = 5; + RemoteStoreEnums.PathType shardPathType = RemoteStoreEnums.PathType.HASHED_PREFIX; + RemoteStoreEnums.PathHashAlgorithm shardPathHashAlgorithm = RemoteStoreEnums.PathHashAlgorithm.FNV_1A_BASE64; + + AssertionError exception = expectThrows( + AssertionError.class, + () -> new SnapshotShardPaths(paths, indexId, indexName, numberOfShards, shardPathType, shardPathHashAlgorithm) + ); + assertTrue(exception.getMessage().contains("indexId must not be empty")); } - public void testFromXContentEmptyPaths() throws IOException { - String json = "{\"paths\":[]}"; - try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { - SnapshotShardPaths indexIdPaths = SnapshotShardPaths.fromXContent(parser); - assertEquals(Collections.emptyList(), indexIdPaths.getPaths()); - } + public void testMissingIndexName() { + List paths = Arrays.asList("/path/to/shard/1", "/path/to/shard/2", "/path/to/shard/3"); + String indexId = "index-id"; + String indexName = ""; + int numberOfShards = 5; + RemoteStoreEnums.PathType shardPathType = RemoteStoreEnums.PathType.HASHED_PREFIX; + RemoteStoreEnums.PathHashAlgorithm shardPathHashAlgorithm = RemoteStoreEnums.PathHashAlgorithm.FNV_1A_BASE64; + + AssertionError exception = expectThrows( + AssertionError.class, + () -> new SnapshotShardPaths(paths, indexId, indexName, numberOfShards, shardPathType, shardPathHashAlgorithm) + ); + assertTrue(exception.getMessage().contains("indexName must not be empty")); } - public void testFromXContentMissingPaths() throws IOException { - String json = "{}"; - try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { - OpenSearchParseException exception = expectThrows( - OpenSearchParseException.class, - () -> SnapshotShardPaths.fromXContent(parser) - ); - assertEquals("Missing [paths] field", exception.getMessage()); - } + public void testMissingNumberOfShards() { + List paths = Arrays.asList("/path/to/shard/1", "/path/to/shard/2", "/path/to/shard/3"); + String indexId = "index-id"; + String indexName = "index-name"; + int numberOfShards = 0; + RemoteStoreEnums.PathType shardPathType = RemoteStoreEnums.PathType.HASHED_PREFIX; + RemoteStoreEnums.PathHashAlgorithm shardPathHashAlgorithm = RemoteStoreEnums.PathHashAlgorithm.FNV_1A_BASE64; + + AssertionError exception = expectThrows( + AssertionError.class, + () -> new SnapshotShardPaths(paths, indexId, indexName, numberOfShards, shardPathType, shardPathHashAlgorithm) + ); + assertTrue(exception.getMessage().contains("numberOfShards must be > 0")); } - public void testFromXContentInvalidJson() throws IOException { - String json = "{\"paths\":\"invalid\"}"; - try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { - OpenSearchParseException exception = expectThrows( - OpenSearchParseException.class, - () -> SnapshotShardPaths.fromXContent(parser) - ); - assertEquals("Expected an array for field [paths]", exception.getMessage()); - } + public void testMissingShardPathType() { + List paths = Arrays.asList("/path/to/shard/1", "/path/to/shard/2", "/path/to/shard/3"); + String indexId = "index-id"; + String indexName = "index-name"; + int numberOfShards = 5; + RemoteStoreEnums.PathType shardPathType = null; + RemoteStoreEnums.PathHashAlgorithm shardPathHashAlgorithm = RemoteStoreEnums.PathHashAlgorithm.FNV_1A_BASE64; + + AssertionError exception = expectThrows( + AssertionError.class, + () -> new SnapshotShardPaths(paths, indexId, indexName, numberOfShards, shardPathType, shardPathHashAlgorithm) + ); + assertTrue(exception.getMessage().contains("shardPathType must not be null")); } - public void testFromXContentInvalidJsonObject() throws IOException { - String json = "invalid"; - try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { - OpenSearchParseException exception = expectThrows( - OpenSearchParseException.class, - () -> SnapshotShardPaths.fromXContent(parser) - ); - assertEquals("Failed to parse SnapshotIndexIdPaths", exception.getMessage()); - } + public void testMissingShardPathHashAlgorithm() { + List paths = Arrays.asList("/path/to/shard/1", "/path/to/shard/2", "/path/to/shard/3"); + String indexId = "index-id"; + String indexName = "index-name"; + int numberOfShards = 5; + RemoteStoreEnums.PathType shardPathType = RemoteStoreEnums.PathType.HASHED_PREFIX; + RemoteStoreEnums.PathHashAlgorithm shardPathHashAlgorithm = null; + + AssertionError exception = expectThrows( + AssertionError.class, + () -> new SnapshotShardPaths(paths, indexId, indexName, numberOfShards, shardPathType, shardPathHashAlgorithm) + ); + assertEquals("shardPathHashAlgorithm must not be null", exception.getMessage()); } - public void testFromXContentNotAnObject() throws IOException { - String json = "[]"; - try (XContentParser parser = createParser(JsonXContent.jsonXContent, json)) { - OpenSearchParseException exception = expectThrows( - OpenSearchParseException.class, - () -> SnapshotShardPaths.fromXContent(parser) - ); - assertEquals("Expected a start object", exception.getMessage()); - } + public void testFromXContent() { + UnsupportedOperationException exception = expectThrows( + UnsupportedOperationException.class, + () -> SnapshotShardPaths.fromXContent(null) + ); + assertEquals("SnapshotShardPaths.fromXContent() is not supported", exception.getMessage()); } } From 173514b9ce3cb2b0947fc8cba5e20f797c335235 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 28 Aug 2024 22:40:48 +0530 Subject: [PATCH 03/12] Address comments and add UTs Signed-off-by: Ashish Singh --- .../blobstore/BlobStoreRepository.java | 17 +++- .../blobstore/BlobStoreRepositoryTests.java | 90 +++++++++++++++++++ 2 files changed, 104 insertions(+), 3 deletions(-) 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 7b7ffaa4b682e..54241a8d28254 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -1302,6 +1302,9 @@ private void executeStaleShardDelete( if (blobName.startsWith(SHALLOW_SNAPSHOT_PREFIX)) { String snapshotUUID = extractShallowSnapshotUUID(blobName).orElseThrow(); String[] parts = blobPath.toArray(); + // For fixed, the parts would look like [,"indices","",""] + // For hashed_prefix, the parts would look like ["j01010001010",,"indices","",""] + // For hashed_infix, the parts would look like [,"j01010001010","indices","",""] int partLength = parts.length; String indexId = parts[partLength - 2]; String shardId = parts[partLength - 1]; @@ -1784,6 +1787,7 @@ private void executeOneStaleIndexDelete( cleanupRemoteStoreLocks(indexEntry, highestGenShardPaths, remoteStoreLockManagerFactory); } + // The below deleteShardData would be no-op for FIXED path type snapshots. DeleteResult deleteResult = deleteShardData(highestGenShardPaths, matchingShardPaths, snapshotShardPaths); deleteResult = deleteResult.add(indexEntry.getValue().delete()); // Deleting the index folder logger.debug("[{}] Cleaned up stale index [{}]", metadata.name(), indexSnId); @@ -1886,7 +1890,8 @@ void releaseRemoteStoreLocksAndCleanup( } /** - * Deletes shard data for the highest generation shard paths. + * Deletes shard data for the highest generation shard paths. This is only applicable for hashed_prefix or hashed_infix + * path types for snapshots. The fixed path will bail out due to highestGenShardPaths being empty. * * @param highestGenShardPaths The optional highest generation shard path * @param matchingShardPaths List of shard paths that match a specific criteria @@ -1953,8 +1958,10 @@ public void finalizeSnapshot( indexMetas, indexMetaIdentifiers ); + // The snapshot shards path would be uploaded for new index ids or index ids where the shard gen count (a.k.a + // number_of_shards) has increased. Set updatedIndexIds = writeNewIndexShardPaths(existingRepositoryData, updatedRepositoryData, snapshotId); - cleanupStaleSnapshotShardPaths(updatedIndexIds); + cleanupRedundantSnapshotShardPaths(updatedIndexIds); writeIndexGen( updatedRepositoryData, repositoryStateId, @@ -2005,7 +2012,11 @@ public void finalizeSnapshot( }, onUpdateFailure); } - private void cleanupStaleSnapshotShardPaths(Set updatedShardPathsIndexIds) { + /** + * This method cleans up the redundant snapshot shard paths file for index ids where the number of shards has increased + * on account of new indexes by same index name being snapshotted that exists already in the repository's snapshots. + */ + private void cleanupRedundantSnapshotShardPaths(Set updatedShardPathsIndexIds) { Set updatedIndexIds = updatedShardPathsIndexIds.stream() .map(s -> s.split(SnapshotShardPaths.DELIMITER)[0]) .collect(Collectors.toSet()); diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java index 79204584ad0ce..c51941cb650ff 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -44,9 +44,11 @@ import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.blobstore.BlobMetadata; import org.opensearch.common.blobstore.DeleteResult; +import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.unit.ByteSizeUnit; +import org.opensearch.core.compress.Compressor; import org.opensearch.core.index.Index; import org.opensearch.core.index.shard.ShardId; import org.opensearch.core.xcontent.NamedXContentRegistry; @@ -62,6 +64,7 @@ import org.opensearch.repositories.Repository; import org.opensearch.repositories.RepositoryData; import org.opensearch.repositories.RepositoryException; +import org.opensearch.repositories.RepositoryStats; import org.opensearch.repositories.ShardGenerations; import org.opensearch.repositories.fs.FsRepository; import org.opensearch.snapshots.SnapshotId; @@ -560,4 +563,91 @@ public void testCleanupStaleIndices() throws Exception { assertEquals("Total bytes deleted should be 150", 150L, combinedResult.bytesDeleted()); assertEquals("Total blobs deleted should be 2", 2, combinedResult.blobsDeleted()); } + + public void testGetMetadata() { + BlobStoreRepository repository = setupRepo(); + RepositoryMetadata metadata = repository.getMetadata(); + assertNotNull(metadata); + assertEquals(metadata.name(), "test-repo"); + assertEquals(metadata.type(), REPO_TYPE); + repository.close(); + } + + public void testGetNamedXContentRegistry() { + BlobStoreRepository repository = setupRepo(); + NamedXContentRegistry registry = repository.getNamedXContentRegistry(); + assertNotNull(registry); + repository.close(); + } + + public void testGetCompressor() { + BlobStoreRepository repository = setupRepo(); + Compressor compressor = repository.getCompressor(); + assertNotNull(compressor); + repository.close(); + } + + public void testGetStats() { + BlobStoreRepository repository = setupRepo(); + RepositoryStats stats = repository.stats(); + assertNotNull(stats); + repository.close(); + } + + public void testGetSnapshotThrottleTimeInNanos() { + BlobStoreRepository repository = setupRepo(); + long throttleTime = repository.getSnapshotThrottleTimeInNanos(); + assertTrue(throttleTime >= 0); + repository.close(); + } + + public void testGetRestoreThrottleTimeInNanos() { + BlobStoreRepository repository = setupRepo(); + long throttleTime = repository.getRestoreThrottleTimeInNanos(); + assertTrue(throttleTime >= 0); + repository.close(); + } + + public void testGetRemoteUploadThrottleTimeInNanos() { + BlobStoreRepository repository = setupRepo(); + long throttleTime = repository.getRemoteUploadThrottleTimeInNanos(); + assertTrue(throttleTime >= 0); + repository.close(); + } + + public void testGetLowPriorityRemoteUploadThrottleTimeInNanos() { + BlobStoreRepository repository = setupRepo(); + long throttleTime = repository.getLowPriorityRemoteUploadThrottleTimeInNanos(); + assertTrue(throttleTime >= 0); + repository.close(); + } + + public void testGetRemoteDownloadThrottleTimeInNanos() { + BlobStoreRepository repository = setupRepo(); + long throttleTime = repository.getRemoteDownloadThrottleTimeInNanos(); + assertTrue(throttleTime >= 0); + repository.close(); + } + + public void testIsReadOnly() { + BlobStoreRepository repository = setupRepo(); + assertFalse(repository.isReadOnly()); + repository.close(); + } + + public void testIsSystemRepository() { + BlobStoreRepository repository = setupRepo(); + assertFalse(repository.isSystemRepository()); + repository.close(); + } + + public void testGetRestrictedSystemRepositorySettings() { + BlobStoreRepository repository = setupRepo(); + List> settings = repository.getRestrictedSystemRepositorySettings(); + assertNotNull(settings); + assertTrue(settings.contains(BlobStoreRepository.SYSTEM_REPOSITORY_SETTING)); + assertTrue(settings.contains(BlobStoreRepository.READONLY_SETTING)); + assertTrue(settings.contains(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY)); + repository.close(); + } } From e097aa9e28c3c0e15cc43de944ff9a8af0e8c357 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 28 Aug 2024 23:09:51 +0530 Subject: [PATCH 04/12] Change default snapshot shard path type to hashed_prefix for testing Signed-off-by: Ashish Singh --- .../opensearch/repositories/blobstore/BlobStoreRepository.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 54241a8d28254..7ad28edfae2e0 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -276,7 +276,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp public static final Setting SHARD_PATH_TYPE = new Setting<>( "shard_path_type", - PathType.FIXED.toString(), + PathType.HASHED_PREFIX.toString(), PathType::parseString ); From 3aa54e593c43b596dabc5f0734e10a0a8f1322d3 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Thu, 29 Aug 2024 13:24:18 +0530 Subject: [PATCH 05/12] Fix failing tests Signed-off-by: Ashish Singh --- .../blobstore/BlobStoreRepository.java | 154 ++++++++++++++---- .../snapshots/SnapshotShardPaths.java | 2 +- .../blobstore/BlobStoreRepositoryTests.java | 6 +- .../blobstore/BlobStoreTestUtil.java | 22 ++- 4 files changed, 145 insertions(+), 39 deletions(-) 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 7ad28edfae2e0..0d325fcb62d0f 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -1125,13 +1125,27 @@ private void doDeleteShardSnapshots( ActionListener.wrap(() -> listener.onResponse(updatedRepoData)), 2 ); + + // We can create map of indexId to ShardInfo based on the old repository data. This is later used in cleanup + // of stale indexes in combination with Snapshot Shard Paths file + Map idToShardInfoMap = repositoryData.getIndices() + .values() + .stream() + .collect( + Collectors.toMap( + IndexId::getId, + indexId -> new ShardInfo(indexId, repositoryData.shardGenerations().getGens(indexId).size()) + ) + ); + cleanupUnlinkedRootAndIndicesBlobs( snapshotIds, foundIndices, rootBlobs, updatedRepoData, remoteStoreLockManagerFactory, - afterCleanupsListener + afterCleanupsListener, + idToShardInfoMap ); asyncCleanupUnlinkedShardLevelBlobs( repositoryData, @@ -1143,13 +1157,25 @@ private void doDeleteShardSnapshots( }, listener::onFailure); } + /** + * Cleans up the indices and data corresponding to all it's shards. + * + * @param deletedSnapshots list of snapshots being deleted + * @param foundIndices indices that are found at [base_path]/indices + * @param rootBlobs the blobs at the [base_path] + * @param updatedRepoData the new repository data after the deletion + * @param remoteStoreLockManagerFactory remote store lock manager factory used for shallow snapshots + * @param listener listener on deletion of the stale indices + * @param idToShardInfoMap map of indexId to ShardInfo + */ private void cleanupUnlinkedRootAndIndicesBlobs( Collection deletedSnapshots, Map foundIndices, Map rootBlobs, RepositoryData updatedRepoData, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, - ActionListener listener + ActionListener listener, + Map idToShardInfoMap ) { cleanupStaleBlobs( deletedSnapshots, @@ -1157,7 +1183,8 @@ private void cleanupUnlinkedRootAndIndicesBlobs( rootBlobs, updatedRepoData, remoteStoreLockManagerFactory, - ActionListener.map(listener, ignored -> null) + ActionListener.map(listener, ignored -> null), + idToShardInfoMap ); } @@ -1522,6 +1549,7 @@ private List> resolveFilesToDelete( * @param rootBlobs all blobs found directly under the repository root * @param newRepoData new repository data that was just written * @param remoteStoreLockManagerFactory RemoteStoreLockManagerFactory to be used for cleaning up remote store lock files. + * @param idToShardInfoMap map of indexId to ShardInfo * @param listener listener to invoke with the combined {@link DeleteResult} of all blobs removed in this operation */ private void cleanupStaleBlobs( @@ -1530,7 +1558,8 @@ private void cleanupStaleBlobs( Map rootBlobs, RepositoryData newRepoData, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, - ActionListener listener + ActionListener listener, + Map idToShardInfoMap ) { final GroupedActionListener groupedListener = new GroupedActionListener<>(ActionListener.wrap(deleteResults -> { DeleteResult deleteResult = DeleteResult.ZERO; @@ -1556,7 +1585,14 @@ private void cleanupStaleBlobs( groupedListener.onResponse(DeleteResult.ZERO); } else { Map snapshotShardPaths = getSnapshotShardPaths(); - cleanupStaleIndices(foundIndices, survivingIndexIds, remoteStoreLockManagerFactory, groupedListener, snapshotShardPaths); + cleanupStaleIndices( + foundIndices, + survivingIndexIds, + remoteStoreLockManagerFactory, + groupedListener, + snapshotShardPaths, + idToShardInfoMap + ); } } @@ -1620,7 +1656,8 @@ public void cleanup( rootBlobs, repositoryData, remoteStoreLockManagerFactory, - ActionListener.map(listener, RepositoryCleanupResult::new) + ActionListener.map(listener, RepositoryCleanupResult::new), + Collections.emptyMap() ), listener::onFailure ) @@ -1713,7 +1750,8 @@ void cleanupStaleIndices( Set survivingIndexIds, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, GroupedActionListener listener, - Map snapshotShardPaths + Map snapshotShardPaths, + Map idToShardInfoMap ) { final GroupedActionListener groupedListener = new GroupedActionListener<>(ActionListener.wrap(deleteResults -> { DeleteResult deleteResult = DeleteResult.ZERO; @@ -1737,7 +1775,13 @@ void cleanupStaleIndices( foundIndices.size() - survivingIndexIds.size() ); for (int i = 0; i < workers; ++i) { - executeOneStaleIndexDelete(staleIndicesToDelete, remoteStoreLockManagerFactory, groupedListener, snapshotShardPaths); + executeOneStaleIndexDelete( + staleIndicesToDelete, + remoteStoreLockManagerFactory, + groupedListener, + snapshotShardPaths, + idToShardInfoMap + ); } } catch (Exception e) { // TODO: We shouldn't be blanket catching and suppressing all exceptions here and instead handle them safely upstream. @@ -1764,13 +1808,15 @@ private static boolean isIndexPresent(ClusterService clusterService, String inde * @param remoteStoreLockManagerFactory Factory for creating remote store lock managers * @param listener Listener for grouped delete actions * @param snapshotShardPaths Map of snapshot shard paths and their metadata + * @param idToShardInfoMap Map of indexId to ShardInfo * @throws InterruptedException if the thread is interrupted while waiting */ private void executeOneStaleIndexDelete( BlockingQueue> staleIndicesToDelete, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory, GroupedActionListener listener, - Map snapshotShardPaths + Map snapshotShardPaths, + Map idToShardInfoMap ) throws InterruptedException { Map.Entry indexEntry = staleIndicesToDelete.poll(0L, TimeUnit.MILLISECONDS); if (indexEntry == null) { @@ -1782,13 +1828,20 @@ private void executeOneStaleIndexDelete( logger.debug("[{}] Found stale index [{}]. Cleaning it up", metadata.name(), indexSnId); List matchingShardPaths = findMatchingShardPaths(indexSnId, snapshotShardPaths); Optional highestGenShardPaths = findHighestGenerationShardPaths(matchingShardPaths); + ShardInfo shardInfo = getShardInfo(highestGenShardPaths, idToShardInfoMap, indexSnId); if (remoteStoreLockManagerFactory != null) { - cleanupRemoteStoreLocks(indexEntry, highestGenShardPaths, remoteStoreLockManagerFactory); + cleanupRemoteStoreLocks(indexEntry, shardInfo, remoteStoreLockManagerFactory); } - // The below deleteShardData would be no-op for FIXED path type snapshots. - DeleteResult deleteResult = deleteShardData(highestGenShardPaths, matchingShardPaths, snapshotShardPaths); + // The below deleteShardData would be no-op for FIXED path type snapshots. Otherwise, this deletes the + // shard level data for the underlying index based on the pathType present in IndexId + DeleteResult deleteResult = deleteShardData(shardInfo); + + // If there are matchingShardPaths, then we delete them after we have deleted the shard data. + deleteResult = deleteResult.add(cleanUpStaleSnapshotShardPathsFile(matchingShardPaths, snapshotShardPaths)); + + // Finally, we delete the [base_path]/indexId folder deleteResult = deleteResult.add(indexEntry.getValue().delete()); // Deleting the index folder logger.debug("[{}] Cleaned up stale index [{}]", metadata.name(), indexSnId); return deleteResult; @@ -1808,7 +1861,13 @@ private void executeOneStaleIndexDelete( logger.warn(new ParameterizedMessage("[{}] Exception during single stale index delete", metadata.name()), e); return DeleteResult.ZERO; } finally { - executeOneStaleIndexDelete(staleIndicesToDelete, remoteStoreLockManagerFactory, listener, snapshotShardPaths); + executeOneStaleIndexDelete( + staleIndicesToDelete, + remoteStoreLockManagerFactory, + listener, + snapshotShardPaths, + idToShardInfoMap + ); } })); } @@ -1842,19 +1901,18 @@ private Optional findHighestGenerationShardPaths(List matchingSh * Cleans up remote store locks for a given index entry. * * @param indexEntry The index entry containing the blob container - * @param highestGenShardPaths The optional highest generation shard path + * @param shardInfo ShardInfo for the IndexId being cleaned up * @param remoteStoreLockManagerFactory Factory for creating remote store lock managers * @throws IOException If an I/O error occurs during the cleanup process */ private void cleanupRemoteStoreLocks( Map.Entry indexEntry, - Optional highestGenShardPaths, + ShardInfo shardInfo, RemoteStoreLockManagerFactory remoteStoreLockManagerFactory ) throws IOException { - if (highestGenShardPaths.isEmpty()) { + if (shardInfo == null) { releaseRemoteStoreLocksAndCleanup(indexEntry.getValue().children(), remoteStoreLockManagerFactory); } else { - ShardInfo shardInfo = SnapshotShardPaths.parseShardPath(highestGenShardPaths.get()); Map shardContainers = new HashMap<>(shardInfo.getShardCount()); for (int i = 0; i < shardInfo.getShardCount(); i++) { shardContainers.put(String.valueOf(i), shardContainer(shardInfo.getIndexId(), i)); @@ -1890,33 +1948,67 @@ void releaseRemoteStoreLocksAndCleanup( } /** - * Deletes shard data for the highest generation shard paths. This is only applicable for hashed_prefix or hashed_infix - * path types for snapshots. The fixed path will bail out due to highestGenShardPaths being empty. + * Deletes shard data for the provided ShardInfo object. * - * @param highestGenShardPaths The optional highest generation shard path - * @param matchingShardPaths List of shard paths that match a specific criteria - * @param snapshotShardPaths Map of snapshot shard paths and their metadata - * @throws IOException If an I/O error occurs during the deletion process + * @param shardInfo The ShardInfo object containing information about the shards to be deleted. + * @return A DeleteResult object representing the result of the deletion operation. + * @throws IOException If an I/O error occurs during the deletion process. */ - private DeleteResult deleteShardData( - Optional highestGenShardPaths, - List matchingShardPaths, - Map snapshotShardPaths - ) throws IOException { - if (highestGenShardPaths.isEmpty()) { + private DeleteResult deleteShardData(ShardInfo shardInfo) throws IOException { + // If the provided ShardInfo is null, return a zero DeleteResult + if (shardInfo == null) { return DeleteResult.ZERO; } - ShardInfo shardInfo = SnapshotShardPaths.parseShardPath(highestGenShardPaths.get()); + // Initialize the DeleteResult with zero values DeleteResult deleteResult = DeleteResult.ZERO; + // Iterate over the shards and delete each shard's data for (int i = 0; i < shardInfo.getShardCount(); i++) { + // Call the delete method on the shardContainer and accumulate the result deleteResult = deleteResult.add(shardContainer(shardInfo.getIndexId(), i).delete()); } + // Return the accumulated DeleteResult + return deleteResult; + } + + /** + * Retrieves the ShardInfo object based on the provided highest generation shard paths, + * index ID, and the mapping of index IDs to ShardInfo objects. + * + * @param highestGenShardPaths The optional highest generation shard path. + * @param idToShardInfoMap A map containing index IDs and their corresponding ShardInfo objects. + * @param indexId The index ID for which the ShardInfo object is needed. + * @return The ShardInfo object with the highest shard count, or null if no ShardInfo is available. + */ + private ShardInfo getShardInfo(Optional highestGenShardPaths, Map idToShardInfoMap, String indexId) { + // Extract the ShardInfo object from the highest generation shard path, if present + ShardInfo shardInfoFromPath = highestGenShardPaths.map(SnapshotShardPaths::parseShardPath).orElse(null); + + // Retrieve the ShardInfo object from the idToShardInfoMap using the indexId + ShardInfo shardInfoFromMap = idToShardInfoMap.get(indexId); + + // If shardInfoFromPath is null, return shardInfoFromMap (which could also be null) + if (shardInfoFromPath == null) { + return shardInfoFromMap; + } + + // If shardInfoFromMap is null, return shardInfoFromPath (which could also be null) + if (shardInfoFromMap == null) { + return shardInfoFromPath; + } + + // If both shardInfoFromPath and shardInfoFromMap are non-null, + // return the ShardInfo object with the higher shard count + return shardInfoFromPath.getShardCount() >= shardInfoFromMap.getShardCount() ? shardInfoFromPath : shardInfoFromMap; + } + + private DeleteResult cleanUpStaleSnapshotShardPathsFile(List matchingShardPaths, Map snapshotShardPaths) + throws IOException { deleteFromContainer(snapshotShardPathBlobContainer(), matchingShardPaths); long totalBytes = matchingShardPaths.stream().mapToLong(s -> snapshotShardPaths.get(s).length()).sum(); - return deleteResult.add(matchingShardPaths.size(), totalBytes); + return new DeleteResult(matchingShardPaths.size(), totalBytes); } @Override diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java index 8fa12b028e0f1..40757b679c32a 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java @@ -126,7 +126,7 @@ public static class ShardInfo { * @param indexId The ID of the index associated with this shard. * @param shardCount The total number of shards for this index. */ - ShardInfo(IndexId indexId, int shardCount) { + public ShardInfo(IndexId indexId, int shardCount) { this.indexId = indexId; this.shardCount = shardCount; } diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java index c51941cb650ff..67c5f14a1c355 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -93,6 +93,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -517,7 +518,7 @@ public void testCleanupStaleIndices() throws Exception { listener.onResponse(result); return null; - }).when(repository).cleanupStaleIndices(any(), any(), any(), any(), any()); + }).when(repository).cleanupStaleIndices(any(), any(), any(), any(), any(), anyMap()); AtomicReference> resultReference = new AtomicReference<>(); CountDownLatch latch = new CountDownLatch(1); @@ -536,7 +537,8 @@ public void testCleanupStaleIndices() throws Exception { survivingIndexIds, mockRemoteStoreLockManagerFactory, listener, - mockSnapshotShardPaths + mockSnapshotShardPaths, + Collections.emptyMap() ); assertTrue("Cleanup did not complete within the expected time", latch.await(30, TimeUnit.SECONDS)); diff --git a/test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java b/test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java index 32f445bf24a41..f000000407346 100644 --- a/test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java +++ b/test/framework/src/main/java/org/opensearch/repositories/blobstore/BlobStoreTestUtil.java @@ -142,7 +142,7 @@ public static void assertConsistency(BlobStoreRepository repository, Executor ex } assertIndexUUIDs(repository, repositoryData); assertSnapshotUUIDs(repository, repositoryData); - assertShardIndexGenerations(blobContainer, repositoryData); + assertShardIndexGenerations(repository, repositoryData); return null; } catch (AssertionError e) { return e; @@ -166,14 +166,12 @@ private static void assertIndexGenerations(BlobContainer repoRoot, long latestGe assertTrue(indexGenerations.length <= 2); } - private static void assertShardIndexGenerations(BlobContainer repoRoot, RepositoryData repositoryData) throws IOException { + private static void assertShardIndexGenerations(BlobStoreRepository repository, RepositoryData repositoryData) throws IOException { final ShardGenerations shardGenerations = repositoryData.shardGenerations(); - final BlobContainer indicesContainer = repoRoot.children().get("indices"); for (IndexId index : shardGenerations.indices()) { final List gens = shardGenerations.getGens(index); if (gens.isEmpty() == false) { - final BlobContainer indexContainer = indicesContainer.children().get(index.getId()); - final Map shardContainers = indexContainer.children(); + final Map shardContainers = getShardContainers(index, repository, repositoryData); for (int i = 0; i < gens.size(); i++) { final String generation = gens.get(i); assertThat(generation, not(ShardGenerations.DELETED_SHARD_GEN)); @@ -190,6 +188,20 @@ private static void assertShardIndexGenerations(BlobContainer repoRoot, Reposito } } + private static Map getShardContainers( + IndexId indexId, + BlobStoreRepository repository, + RepositoryData repositoryData + ) { + final Map shardContainers = new HashMap<>(); + int shardCount = repositoryData.shardGenerations().getGens(indexId).size(); + for (int i = 0; i < shardCount; i++) { + final BlobContainer shardContainer = repository.shardContainer(indexId, i); + shardContainers.put(String.valueOf(i), shardContainer); + } + return shardContainers; + } + private static void assertIndexUUIDs(BlobStoreRepository repository, RepositoryData repositoryData) throws IOException { final List expectedIndexUUIDs = repositoryData.getIndices() .values() From 6446b031367562757eb8806622f186969621d5f2 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Fri, 30 Aug 2024 00:51:20 +0530 Subject: [PATCH 06/12] Introduce single method for all create/put repository calls Signed-off-by: Ashish Singh --- .../url/URLSnapshotRestoreIT.java | 33 ++----- .../AzureStorageCleanupThirdPartyTests.java | 17 ++-- .../GoogleCloudStorageThirdPartyTests.java | 16 +--- .../hdfs/HdfsRepositoryTests.java | 22 ++--- .../repositories/hdfs/HdfsTests.java | 57 ++++-------- .../s3/S3RepositoryThirdPartyTests.java | 9 +- .../s3/RepositoryCredentialsTests.java | 11 +-- .../repositories/RepositoryBlocksIT.java | 55 ++++------- .../cluster/snapshots/SnapshotBlocksIT.java | 9 +- .../indices/create/RemoteCloneIndexIT.java | 2 +- .../decider/DiskThresholdDeciderIT.java | 9 +- .../cluster/shards/ClusterShardLimitIT.java | 3 +- .../index/store/CorruptedFileIT.java | 35 +++---- .../indices/IndicesOptionsIntegrationIT.java | 18 +--- .../indices/recovery/IndexRecoveryIT.java | 9 +- .../MigrationBaseTestCase.java | 4 +- .../RemoteStoreMigrationSettingsUpdateIT.java | 7 +- .../RemoteStoreMigrationTestCase.java | 13 +-- .../RemoteStoreBaseIntegTestCase.java | 4 +- .../RemoteStoreClusterStateRestoreIT.java | 9 +- .../opensearch/remotestore/RemoteStoreIT.java | 4 +- .../RemoteStoreRepositoryRegistrationIT.java | 22 ++--- .../remotestore/RemoteStoreRestoreIT.java | 19 +--- .../multipart/RemoteStoreMultipartIT.java | 20 ++-- .../repositories/RepositoriesServiceIT.java | 16 +--- .../fs/FsBlobStoreRepositoryIT.java | 27 ++---- .../CorruptedBlobStoreRepositoryIT.java | 37 +++----- .../DedicatedClusterSnapshotRestoreIT.java | 33 ++++--- .../snapshots/MultiClusterRepoAccessIT.java | 15 +-- .../opensearch/snapshots/RepositoriesIT.java | 91 ++++++++----------- .../RepositoryFilterUserMetadataIT.java | 16 +--- .../SharedClusterSnapshotRestoreIT.java | 72 ++++----------- .../snapshots/SystemRepositoryIT.java | 21 +---- .../BlobStoreRepositoryHelperTests.java | 23 ++--- .../blobstore/BlobStoreRepositoryTests.java | 53 +++-------- .../snapshots/SnapshotResiliencyTests.java | 9 +- ...earchBlobStoreRepositoryIntegTestCase.java | 4 +- .../AbstractSnapshotIntegTestCase.java | 11 --- .../test/OpenSearchIntegTestCase.java | 70 +++++++++++++- 39 files changed, 317 insertions(+), 588 deletions(-) diff --git a/modules/repository-url/src/internalClusterTest/java/org/opensearch/repositories/url/URLSnapshotRestoreIT.java b/modules/repository-url/src/internalClusterTest/java/org/opensearch/repositories/url/URLSnapshotRestoreIT.java index 9c61bca316a56..fd0c6b309c4fd 100644 --- a/modules/repository-url/src/internalClusterTest/java/org/opensearch/repositories/url/URLSnapshotRestoreIT.java +++ b/modules/repository-url/src/internalClusterTest/java/org/opensearch/repositories/url/URLSnapshotRestoreIT.java @@ -67,19 +67,11 @@ public void testUrlRepository() throws Exception { logger.info("--> creating repository"); Path repositoryLocation = randomRepoPath(); - assertAcked( - client.admin() - .cluster() - .preparePutRepository("test-repo") - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), repositoryLocation) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - ) - ); - + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), repositoryLocation) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES); + createRepository("test-repo", FsRepository.TYPE, settings); createIndex("test-idx"); ensureGreen(); @@ -115,17 +107,10 @@ public void testUrlRepository() throws Exception { cluster().wipeIndices("test-idx"); logger.info("--> create read-only URL repository"); - assertAcked( - client.admin() - .cluster() - .preparePutRepository("url-repo") - .setType(URLRepository.TYPE) - .setSettings( - Settings.builder() - .put(URLRepository.URL_SETTING.getKey(), repositoryLocation.toUri().toURL().toString()) - .put("list_directories", randomBoolean()) - ) - ); + Settings.Builder settingsBuilder = Settings.builder() + .put(URLRepository.URL_SETTING.getKey(), repositoryLocation.toUri().toURL().toString()) + .put("list_directories", randomBoolean()); + createRepository("url-repo", URLRepository.TYPE, settingsBuilder); logger.info("--> restore index after deletion"); RestoreSnapshotResponse restoreSnapshotResponse = client.admin() .cluster() diff --git a/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java b/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java index 176e60a667aef..522940b6c7729 100644 --- a/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java +++ b/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java @@ -53,6 +53,7 @@ import java.util.Collection; import java.util.function.Supplier; +import org.opensearch.test.OpenSearchIntegTestCase; import reactor.core.scheduler.Schedulers; import static org.hamcrest.Matchers.blankOrNullString; @@ -103,17 +104,11 @@ protected SecureSettings credentials() { @Override protected void createRepository(String repoName) { - AcknowledgedResponse putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository(repoName) - .setType("azure") - .setSettings( - Settings.builder() - .put("container", System.getProperty("test.azure.container")) - .put("base_path", System.getProperty("test.azure.base")) - ) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settings = Settings.builder() + .put("container", System.getProperty("test.azure.container")) + .put("base_path", System.getProperty("test.azure.base")); + + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), repoName, "azure", settings); if (Strings.hasText(System.getProperty("test.azure.sas_token"))) { ensureSasTokenPermissions(); } diff --git a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java index 1e11b1d111d8f..727a923104338 100644 --- a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java +++ b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java @@ -39,6 +39,7 @@ import org.opensearch.core.common.Strings; import org.opensearch.plugins.Plugin; import org.opensearch.repositories.AbstractThirdPartyRepositoryTestCase; +import org.opensearch.test.OpenSearchIntegTestCase; import java.util.Base64; import java.util.Collection; @@ -84,16 +85,9 @@ protected SecureSettings credentials() { @Override protected void createRepository(final String repoName) { - AcknowledgedResponse putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("gcs") - .setSettings( - Settings.builder() - .put("bucket", System.getProperty("test.google.bucket")) - .put("base_path", System.getProperty("test.google.base", "/")) - ) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settings = Settings.builder() + .put("bucket", System.getProperty("test.google.bucket")) + .put("base_path", System.getProperty("test.google.base", "/")); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo", "gcs", settings); } } diff --git a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java index ab10691240649..fd6f6ffb74741 100644 --- a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java +++ b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java @@ -40,6 +40,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.plugins.Plugin; import org.opensearch.repositories.AbstractThirdPartyRepositoryTestCase; +import org.opensearch.test.OpenSearchIntegTestCase; import java.util.Collection; @@ -61,20 +62,13 @@ protected SecureSettings credentials() { @Override protected void createRepository(String repoName) { - AcknowledgedResponse putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository(repoName) - .setType("hdfs") - .setSettings( - Settings.builder() - .put("uri", "hdfs:///") - .put("conf.fs.AbstractFileSystem.hdfs.impl", TestingFs.class.getName()) - .put("path", "foo") - .put("chunk_size", randomIntBetween(100, 1000) + "k") - .put("compress", randomBoolean()) - ) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settings = Settings.builder() + .put("uri", "hdfs:///") + .put("conf.fs.AbstractFileSystem.hdfs.impl", TestingFs.class.getName()) + .put("path", "foo") + .put("chunk_size", randomIntBetween(100, 1000) + "k") + .put("compress", randomBoolean()); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), repoName, "hdfs", settings); } // HDFS repository doesn't have precise cleanup stats so we only check whether or not any blobs were removed diff --git a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java index ce456f26af3a4..99cab6a197b41 100644 --- a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java +++ b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java @@ -45,6 +45,7 @@ import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.repositories.blobstore.BlobStoreTestUtil; import org.opensearch.snapshots.SnapshotState; +import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.OpenSearchSingleNodeTestCase; import org.opensearch.threadpool.ThreadPool; @@ -63,21 +64,13 @@ protected Collection> getPlugins() { public void testSimpleWorkflow() { Client client = client(); - - AcknowledgedResponse putRepositoryResponse = client.admin() - .cluster() - .preparePutRepository("test-repo") - .setType("hdfs") - .setSettings( - Settings.builder() - .put("uri", "hdfs:///") - .put("conf.fs.AbstractFileSystem.hdfs.impl", TestingFs.class.getName()) - .put("path", "foo") - .put("chunk_size", randomIntBetween(100, 1000) + "k") - .put("compress", randomBoolean()) - ) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settings = Settings.builder() + .put("uri", "hdfs:///") + .put("conf.fs.AbstractFileSystem.hdfs.impl", TestingFs.class.getName()) + .put("path", "foo") + .put("chunk_size", randomIntBetween(100, 1000) + "k") + .put("compress", randomBoolean()); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), "test-repo", "hdfs", settings); createIndex("test-idx-1"); createIndex("test-idx-2"); @@ -168,7 +161,7 @@ public void testSimpleWorkflow() { public void testMissingUri() { try { - client().admin().cluster().preparePutRepository("test-repo").setType("hdfs").setSettings(Settings.EMPTY).get(); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo", "hdfs", Settings.builder()); fail(); } catch (RepositoryException e) { assertTrue(e.getCause() instanceof IllegalArgumentException); @@ -178,12 +171,8 @@ public void testMissingUri() { public void testEmptyUri() { try { - client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("hdfs") - .setSettings(Settings.builder().put("uri", "/path").build()) - .get(); + Settings.Builder settings = Settings.builder().put("uri", "/path"); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo", "hdfs", settings); fail(); } catch (RepositoryException e) { assertTrue(e.getCause() instanceof IllegalArgumentException); @@ -193,12 +182,8 @@ public void testEmptyUri() { public void testNonHdfsUri() { try { - client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("hdfs") - .setSettings(Settings.builder().put("uri", "file:///").build()) - .get(); + Settings.Builder settings = Settings.builder().put("uri", "file:///"); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo", "hdfs", settings); fail(); } catch (RepositoryException e) { assertTrue(e.getCause() instanceof IllegalArgumentException); @@ -208,12 +193,8 @@ public void testNonHdfsUri() { public void testPathSpecifiedInHdfs() { try { - client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("hdfs") - .setSettings(Settings.builder().put("uri", "hdfs:///some/path").build()) - .get(); + Settings.Builder settings = Settings.builder().put("uri", "hdfs:///some/path"); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo", "hdfs", settings); fail(); } catch (RepositoryException e) { assertTrue(e.getCause() instanceof IllegalArgumentException); @@ -223,12 +204,8 @@ public void testPathSpecifiedInHdfs() { public void testMissingPath() { try { - client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("hdfs") - .setSettings(Settings.builder().put("uri", "hdfs:///").build()) - .get(); + Settings.Builder settings = Settings.builder().put("uri", "hdfs:///"); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo", "hdfs", settings); fail(); } catch (RepositoryException e) { assertTrue(e.getCause() instanceof IllegalArgumentException); diff --git a/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java b/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java index f7a84864a8569..4ba46c1632072 100644 --- a/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java +++ b/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java @@ -31,6 +31,7 @@ package org.opensearch.repositories.s3; +import org.opensearch.test.OpenSearchIntegTestCase; import software.amazon.awssdk.services.s3.model.StorageClass; import org.opensearch.action.support.master.AcknowledgedResponse; @@ -111,13 +112,7 @@ protected void createRepository(String repoName) { settings.put("storage_class", storageClass); } } - AcknowledgedResponse putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("s3") - .setSettings(settings) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo", "s3", settings); } @Override diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java index 573a4f3f51a41..c1b7d82dab6c3 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java @@ -32,6 +32,7 @@ package org.opensearch.repositories.s3; +import org.opensearch.test.OpenSearchIntegTestCase; import software.amazon.awssdk.auth.credentials.AwsCredentials; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.services.s3.DelegatingS3Client; @@ -277,14 +278,8 @@ public void sendResponse(RestResponse response) { } private void createRepository(final String name, final Settings repositorySettings) { - assertAcked( - client().admin() - .cluster() - .preparePutRepository(name) - .setType(S3Repository.TYPE) - .setVerify(false) - .setSettings(repositorySettings) - ); + Settings.Builder settings = Settings.builder().put(repositorySettings); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), name, S3Repository.TYPE, false, settings); } /** diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java index 36fe3748e9d10..dc1ace7c43e40 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java @@ -55,13 +55,16 @@ public void testPutRepositoryWithBlocks() { logger.info("--> registering a repository is blocked when the cluster is read only"); try { setClusterReadOnly(true); + Settings.Builder settings = Settings.builder().put("location", randomRepoPath()); assertBlocked( - client().admin() - .cluster() - .preparePutRepository("test-repo-blocks") - .setType("fs") - .setVerify(false) - .setSettings(Settings.builder().put("location", randomRepoPath())), + OpenSearchIntegTestCase.putRepositoryRequestBuilder( + client().admin().cluster(), + "test-repo-blocks", + "fs", + false, + settings, + null + ), Metadata.CLUSTER_READ_ONLY_BLOCK ); } finally { @@ -69,25 +72,13 @@ public void testPutRepositoryWithBlocks() { } logger.info("--> registering a repository is allowed when the cluster is not read only"); - assertAcked( - client().admin() - .cluster() - .preparePutRepository("test-repo-blocks") - .setType("fs") - .setVerify(false) - .setSettings(Settings.builder().put("location", randomRepoPath())) - ); + Settings.Builder settings = Settings.builder().put("location", randomRepoPath()); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo-blocks", "fs", false, settings); } public void testVerifyRepositoryWithBlocks() { - assertAcked( - client().admin() - .cluster() - .preparePutRepository("test-repo-blocks") - .setType("fs") - .setVerify(false) - .setSettings(Settings.builder().put("location", randomRepoPath())) - ); + Settings.Builder settings = Settings.builder().put("location", randomRepoPath()); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo-blocks", "fs", false, settings); // This test checks that the Get Repository operation is never blocked, even if the cluster is read only. try { @@ -104,14 +95,8 @@ public void testVerifyRepositoryWithBlocks() { } public void testDeleteRepositoryWithBlocks() { - assertAcked( - client().admin() - .cluster() - .preparePutRepository("test-repo-blocks") - .setType("fs") - .setVerify(false) - .setSettings(Settings.builder().put("location", randomRepoPath())) - ); + Settings.Builder settings = Settings.builder().put("location", randomRepoPath()); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo-blocks", "fs", false, settings); logger.info("--> deleting a repository is blocked when the cluster is read only"); try { @@ -126,14 +111,8 @@ public void testDeleteRepositoryWithBlocks() { } public void testGetRepositoryWithBlocks() { - assertAcked( - client().admin() - .cluster() - .preparePutRepository("test-repo-blocks") - .setType("fs") - .setVerify(false) - .setSettings(Settings.builder().put("location", randomRepoPath())) - ); + Settings.Builder settings = Settings.builder().put("location", randomRepoPath()); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), "test-repo-blocks", "fs", false, settings); // This test checks that the Get Repository operation is never blocked, even if the cluster is read only. try { diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java index 78fb01b07b6b1..0f29f02b284a4 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java @@ -80,13 +80,8 @@ protected void setUpRepository() throws Exception { logger.info("--> register a repository"); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(REPOSITORY_NAME) - .setType("fs") - .setSettings(Settings.builder().put("location", randomRepoPath())) - ); + Settings.Builder settings = Settings.builder().put("location", randomRepoPath()); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), REPOSITORY_NAME, "fs", settings); logger.info("--> verify the repository"); VerifyRepositoryResponse verifyResponse = client().admin().cluster().prepareVerifyRepository(REPOSITORY_NAME).get(); diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/RemoteCloneIndexIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/RemoteCloneIndexIT.java index 009f5111078de..abce2fc878f27 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/RemoteCloneIndexIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/indices/create/RemoteCloneIndexIT.java @@ -224,7 +224,7 @@ protected void setLowPriorityUploadRate(String repoName, String value) throws Ex Settings.Builder settings = Settings.builder() .put("location", rmd.settings().get("location")) .put("max_remote_low_priority_upload_bytes_per_sec", value); - assertAcked(client().admin().cluster().preparePutRepository(repoName).setType(rmd.type()).setSettings(settings).get()); + createRepository(repoName, rmd.type(), settings); } public void testCreateCloneIndexFailure() throws ExecutionException, InterruptedException { diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java index cc8747e5f5666..fc126613ce34e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java @@ -364,13 +364,8 @@ public void testRestoreSnapshotAllocationDoesNotExceedWatermark() throws Excepti final String dataNodeName = internalCluster().startDataOnlyNode(); ensureStableCluster(3); - assertAcked( - client().admin() - .cluster() - .preparePutRepository("repo") - .setType(FsRepository.TYPE) - .setSettings(Settings.builder().put("location", randomRepoPath()).put("compress", randomBoolean())) - ); + Settings.Builder settings = Settings.builder().put("location", randomRepoPath()).put("compress", randomBoolean()); + createRepository("repo", FsRepository.TYPE, settings); final InternalClusterInfoService clusterInfoService = (InternalClusterInfoService) internalCluster() .getCurrentClusterManagerNodeInstance(ClusterInfoService.class); diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java index 5eef7074e1dd6..3718dce538053 100644 --- a/server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java @@ -494,8 +494,7 @@ public void testRestoreSnapshotOverLimit() { repoSettings.put("location", randomRepoPath()); repoSettings.put("compress", randomBoolean()); repoSettings.put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES); - - assertAcked(client.admin().cluster().preparePutRepository("test-repo").setType("fs").setSettings(repoSettings.build())); + createRepository("test-repo", "fs", repoSettings); int dataNodes = client().admin().cluster().prepareState().get().getState().getNodes().getDataNodes().size(); ShardCounts counts = ShardCounts.forDataNodeCount(dataNodes); diff --git a/server/src/internalClusterTest/java/org/opensearch/index/store/CorruptedFileIT.java b/server/src/internalClusterTest/java/org/opensearch/index/store/CorruptedFileIT.java index f46f413f4d23f..3ee506f58a9d7 100644 --- a/server/src/internalClusterTest/java/org/opensearch/index/store/CorruptedFileIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/index/store/CorruptedFileIT.java @@ -577,18 +577,12 @@ public void testCorruptFileThenSnapshotAndRestore() throws ExecutionException, I // the other problem here why we can't corrupt segments.X files is that the snapshot flushes again before // it snapshots and that will write a new segments.X+1 file logger.info("--> creating repository"); - assertAcked( - client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("fs") - .setSettings( - Settings.builder() - .put("location", randomRepoPath().toAbsolutePath()) - .put("compress", randomBoolean()) - .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", randomRepoPath().toAbsolutePath()) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES); + createRepository("test-repo", "fs", settings); + logger.info("--> snapshot"); final CreateSnapshotResponse createSnapshotResponse = client().admin() .cluster() @@ -761,18 +755,11 @@ public void testPrimaryCorruptionDuringReplicationDoesNotFailReplicaShard() thro // Create a snapshot repository. This repo is used to take a snapshot after // corrupting a file, which causes the node to notice the corrupt data and // close the shard. - assertAcked( - client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("fs") - .setSettings( - Settings.builder() - .put("location", randomRepoPath().toAbsolutePath()) - .put("compress", randomBoolean()) - .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", randomRepoPath().toAbsolutePath()) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES); + createRepository("test-repo", "fs", settings); client().prepareIndex("test").setSource("field", "value").execute(); indexingInFlight.await(); diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesOptionsIntegrationIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesOptionsIntegrationIT.java index 06d2d2a90de87..0d3c8307c060f 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/IndicesOptionsIntegrationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/IndicesOptionsIntegrationIT.java @@ -51,7 +51,6 @@ import org.opensearch.action.search.SearchRequestBuilder; import org.opensearch.action.search.SearchResponse; import org.opensearch.action.support.IndicesOptions; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; @@ -271,14 +270,8 @@ public void testSpecifiedIndexUnavailableSnapshotRestore() throws Exception { createIndex("test1"); ensureGreen("test1"); waitForRelocation(); + createRepository("dummy-repo", "fs", Settings.builder().put("location", randomRepoPath())); - AcknowledgedResponse putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository("dummy-repo") - .setType("fs") - .setSettings(Settings.builder().put("location", randomRepoPath())) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); client().admin().cluster().prepareCreateSnapshot("dummy-repo", "snap1").setWaitForCompletion(true).get(); verify(snapshot("snap2", "test1", "test2"), true); @@ -391,13 +384,8 @@ public void testWildcardBehaviourSnapshotRestore() throws Exception { ensureGreen("foobar"); waitForRelocation(); - AcknowledgedResponse putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository("dummy-repo") - .setType("fs") - .setSettings(Settings.builder().put("location", randomRepoPath())) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + createRepository("dummy-repo", "fs", Settings.builder().put("location", randomRepoPath())); + client().admin().cluster().prepareCreateSnapshot("dummy-repo", "snap1").setWaitForCompletion(true).get(); IndicesOptions options = IndicesOptions.fromOptions(false, false, true, false); diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexRecoveryIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexRecoveryIT.java index cf93a432d0371..68b29851c6c04 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexRecoveryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexRecoveryIT.java @@ -741,14 +741,7 @@ public void testSnapshotRecovery() throws Exception { String nodeA = internalCluster().startNode(); logger.info("--> create repository"); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(REPO_NAME) - .setType("fs") - .setSettings(Settings.builder().put("location", randomRepoPath()).put("compress", false)) - .get() - ); + createRepository(REPO_NAME, "fs", Settings.builder().put("location", randomRepoPath()).put("compress", false)); ensureGreen(); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotemigration/MigrationBaseTestCase.java b/server/src/internalClusterTest/java/org/opensearch/remotemigration/MigrationBaseTestCase.java index e4e681a5433b5..17a9c3ddbe317 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotemigration/MigrationBaseTestCase.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotemigration/MigrationBaseTestCase.java @@ -108,9 +108,7 @@ protected void setFailRate(String repoName, int value) throws ExecutionException Settings.Builder settings = Settings.builder() .put("location", rmd.settings().get("location")) .put(REPOSITORIES_FAILRATE_SETTING.getKey(), value); - assertAcked( - client().admin().cluster().preparePutRepository(repoName).setType(ReloadableFsRepository.TYPE).setSettings(settings).get() - ); + createRepository(repoName, ReloadableFsRepository.TYPE, settings); } public void initDocRepToRemoteMigration() { diff --git a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java b/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java index 377bd9529ca7a..c701a8d92c336 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java @@ -21,7 +21,6 @@ import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode.MIXED; import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode.STRICT; import static org.opensearch.node.remotestore.RemoteStoreNodeService.Direction.REMOTE_STORE; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) public class RemoteStoreMigrationSettingsUpdateIT extends RemoteStoreMigrationShardAllocationBaseTestCase { @@ -92,11 +91,7 @@ public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMix String snapshotName = "test-snapshot"; String snapshotRepoName = "test-restore-snapshot-repo"; Path snapshotRepoNameAbsolutePath = randomRepoPath().toAbsolutePath(); - assertAcked( - clusterAdmin().preparePutRepository(snapshotRepoName) - .setType("fs") - .setSettings(Settings.builder().put("location", snapshotRepoNameAbsolutePath)) - ); + createRepository(snapshotRepoName, "fs", Settings.builder().put("location", snapshotRepoNameAbsolutePath)); logger.info("Create snapshot of non remote stored backed index"); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationTestCase.java b/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationTestCase.java index 4d37b2a1feb88..7d29e5d328492 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationTestCase.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationTestCase.java @@ -116,15 +116,10 @@ public void testNoShallowSnapshotInMixedMode() throws Exception { logger.info("Create shallow snapshot setting enabled repo"); String shallowSnapshotRepoName = "shallow-snapshot-repo-name"; Path shallowSnapshotRepoPath = randomRepoPath(); - assertAcked( - clusterAdmin().preparePutRepository(shallowSnapshotRepoName) - .setType("fs") - .setSettings( - Settings.builder() - .put("location", shallowSnapshotRepoPath) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), Boolean.TRUE) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", shallowSnapshotRepoPath) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), Boolean.TRUE); + createRepository(shallowSnapshotRepoName, "fs", settings); logger.info("Verify shallow snapshot creation"); final String snapshot1 = "snapshot1"; diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java index f83ae3e0ca820..ba06bb463e5a8 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java @@ -190,9 +190,7 @@ protected void setFailRate(String repoName, int value) throws ExecutionException Settings.Builder settings = Settings.builder() .put("location", rmd.settings().get("location")) .put(REPOSITORIES_FAILRATE_SETTING.getKey(), value); - assertAcked( - client().admin().cluster().preparePutRepository(repoName).setType(ReloadableFsRepository.TYPE).setSettings(settings).get() - ); + createRepository(repoName, ReloadableFsRepository.TYPE, settings); } public Settings indexSettings() { diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java index 11260e0914dc5..c165e5c6727d6 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java @@ -476,14 +476,7 @@ public void testFullClusterRestoreGlobalMetadata() throws Exception { private Path registerCustomRepository() { Path path = randomRepoPath(); - assertAcked( - client().admin() - .cluster() - .preparePutRepository("custom-repo") - .setType("fs") - .setSettings(Settings.builder().put("location", path).put("compress", false)) - .get() - ); + createRepository("custom-repo", "fs", Settings.builder().put("location", path).put("compress", false)); return path; } diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java index 194dce5f4a57a..a82aaf49d7e60 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java @@ -500,9 +500,7 @@ public void testRestoreSnapshotToIndexWithSameNameDifferentUUID() throws Excepti List dataNodes = internalCluster().startDataOnlyNodes(2); Path absolutePath = randomRepoPath().toAbsolutePath(); - assertAcked( - clusterAdmin().preparePutRepository("test-repo").setType("fs").setSettings(Settings.builder().put("location", absolutePath)) - ); + createRepository("test-repo", "fs", Settings.builder().put("location", absolutePath)); logger.info("--> Create index and ingest 50 docs"); createIndex(INDEX_NAME, remoteStoreIndexSettings(1)); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java index b0827dcfe4892..5e8761d837195 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java @@ -131,13 +131,14 @@ public void testMultiNodeClusterRandomNodeRecoverNetworkIsolationPostNonRestrict .get(0); Settings.Builder updatedSettings = Settings.builder().put(repositoryMetadata.settings()).put("chunk_size", new ByteSizeValue(20)); updatedSettings.remove("system_repository"); - - client.admin() - .cluster() - .preparePutRepository(repositoryMetadata.name()) - .setType(repositoryMetadata.type()) - .setSettings(updatedSettings) - .get(); + OpenSearchIntegTestCase.putRepositoryRequestBuilder( + client.admin().cluster(), + repositoryMetadata.name(), + repositoryMetadata.type(), + true, + updatedSettings, + null + ).get(); ensureStableCluster(3, nodesInOneSide.stream().findAny().get()); networkDisruption.stopDisrupting(); @@ -161,12 +162,7 @@ public void testNodeRestartPostNonRestrictedSettingsUpdate() throws Exception { Settings.Builder updatedSettings = Settings.builder().put(repositoryMetadata.settings()).put("chunk_size", new ByteSizeValue(20)); updatedSettings.remove("system_repository"); - client.admin() - .cluster() - .preparePutRepository(repositoryMetadata.name()) - .setType(repositoryMetadata.type()) - .setSettings(updatedSettings) - .get(); + createRepository(repositoryMetadata.name(), repositoryMetadata.type(), updatedSettings); internalCluster().restartRandomDataNode(); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRestoreIT.java index bd84ab026dcea..1a1c7c0cd0e1a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRestoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRestoreIT.java @@ -480,15 +480,7 @@ public void testRateLimitedRemoteDownloads() throws Exception { Settings.Builder settings = Settings.builder(); settingsMap.entrySet().forEach(entry -> settings.put(entry.getKey(), entry.getValue())); settings.put("location", segmentRepoPath).put("max_remote_download_bytes_per_sec", 4, ByteSizeUnit.KB); - - assertAcked( - client().admin() - .cluster() - .preparePutRepository(REPOSITORY_NAME) - .setType(ReloadableFsRepository.TYPE) - .setSettings(settings) - .get() - ); + createRepository(REPOSITORY_NAME, ReloadableFsRepository.TYPE, settings); for (RepositoriesService repositoriesService : internalCluster().getDataNodeInstances(RepositoriesService.class)) { Repository segmentRepo = repositoriesService.repository(REPOSITORY_NAME); @@ -517,14 +509,7 @@ public void testRateLimitedRemoteDownloads() throws Exception { // revert repo metadata to pass asserts on repo metadata vs. node attrs during teardown // https://github.com/opensearch-project/OpenSearch/pull/9569#discussion_r1345668700 settings.remove("max_remote_download_bytes_per_sec"); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(REPOSITORY_NAME) - .setType(ReloadableFsRepository.TYPE) - .setSettings(settings) - .get() - ); + createRepository(REPOSITORY_NAME, ReloadableFsRepository.TYPE, settings); for (RepositoriesService repositoriesService : internalCluster().getDataNodeInstances(RepositoriesService.class)) { Repository segmentRepo = repositoriesService.repository(REPOSITORY_NAME); assertNull(segmentRepo.getMetadata().settings().get("max_remote_download_bytes_per_sec")); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/multipart/RemoteStoreMultipartIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/multipart/RemoteStoreMultipartIT.java index 3dfde6f472525..99cc58848a04a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/multipart/RemoteStoreMultipartIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/multipart/RemoteStoreMultipartIT.java @@ -31,7 +31,6 @@ import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_SETTINGS_ATTRIBUTE_KEY_PREFIX; import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; @@ -119,19 +118,12 @@ public void testRateLimitedRemoteUploads() throws Exception { internalCluster().startNode(clusterSettings.build()); Client client = client(); logger.info("--> updating repository"); - assertAcked( - client.admin() - .cluster() - .preparePutRepository(REPOSITORY_NAME) - .setType(MockFsRepositoryPlugin.TYPE) - .setSettings( - Settings.builder() - .put("location", repositoryLocation) - .put("compress", compress) - .put("max_remote_upload_bytes_per_sec", "1kb") - .put("chunk_size", 100, ByteSizeUnit.BYTES) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", repositoryLocation) + .put("compress", compress) + .put("max_remote_upload_bytes_per_sec", "1kb") + .put("chunk_size", 100, ByteSizeUnit.BYTES); + createRepository(REPOSITORY_NAME, MockFsRepositoryPlugin.TYPE, settings); createIndex(INDEX_NAME, remoteStoreIndexSettings(0)); ensureGreen(); diff --git a/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java b/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java index b8415f4b41815..7022c6ae3b956 100644 --- a/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java @@ -45,7 +45,6 @@ import java.util.Collection; import java.util.Collections; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; @@ -70,10 +69,7 @@ public void testUpdateRepository() { .next(); final Settings.Builder repoSettings = Settings.builder().put("location", randomRepoPath()); - - assertAcked( - client.admin().cluster().preparePutRepository(repositoryName).setType(FsRepository.TYPE).setSettings(repoSettings).get() - ); + createRepository(repositoryName, FsRepository.TYPE, repoSettings); final GetRepositoriesResponse originalGetRepositoriesResponse = client.admin() .cluster() @@ -91,9 +87,7 @@ public void testUpdateRepository() { final boolean updated = randomBoolean(); final String updatedRepositoryType = updated ? "mock" : FsRepository.TYPE; - assertAcked( - client.admin().cluster().preparePutRepository(repositoryName).setType(updatedRepositoryType).setSettings(repoSettings).get() - ); + createRepository(repositoryName, updatedRepositoryType, repoSettings); final GetRepositoriesResponse updatedGetRepositoriesResponse = client.admin() .cluster() @@ -112,12 +106,8 @@ public void testUpdateRepository() { public void testSystemRepositoryCantBeCreated() { internalCluster(); final String repositoryName = "test-repo"; - final Client client = client(); final Settings.Builder repoSettings = Settings.builder().put("system_repository", true).put("location", randomRepoPath()); - assertThrows( - RepositoryException.class, - () -> client.admin().cluster().preparePutRepository(repositoryName).setType(FsRepository.TYPE).setSettings(repoSettings).get() - ); + assertThrows(RepositoryException.class, () -> createRepository(repositoryName, FsRepository.TYPE, repoSettings)); } } diff --git a/server/src/internalClusterTest/java/org/opensearch/repositories/fs/FsBlobStoreRepositoryIT.java b/server/src/internalClusterTest/java/org/opensearch/repositories/fs/FsBlobStoreRepositoryIT.java index 9057ef900efbd..34075b78e98af 100644 --- a/server/src/internalClusterTest/java/org/opensearch/repositories/fs/FsBlobStoreRepositoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/repositories/fs/FsBlobStoreRepositoryIT.java @@ -76,19 +76,11 @@ public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOExce 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) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", repoPath) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES); + createRepository(repoName, "fs", settings); final String indexName = randomName(); int docCount = iterations(10, 1000); @@ -112,14 +104,7 @@ public void testMissingDirectoriesNotCreatedInReadonlyRepository() throws IOExce IOUtils.rm(deletedPath); } assertFalse(Files.exists(deletedPath)); - - assertAcked( - client().admin() - .cluster() - .preparePutRepository(repoName) - .setType("fs") - .setSettings(Settings.builder().put("location", repoPath).put("readonly", true)) - ); + createRepository(repoName, "fs", Settings.builder().put("location", repoPath).put("readonly", true)); final OpenSearchException exception = expectThrows( OpenSearchException.class, diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java index e685aaa52df00..a9d4242ec1bbf 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java @@ -125,18 +125,11 @@ public void testConcurrentlyChangeRepositoryContents() throws Exception { assertAcked(client.admin().cluster().prepareDeleteRepository(repoName)); logger.info("--> recreate repository"); - assertAcked( - client.admin() - .cluster() - .preparePutRepository(repoName) - .setType("fs") - .setSettings( - Settings.builder() - .put("location", repo) - .put("compress", false) - .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", repo) + .put("compress", false) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES); + createRepository(repoName, "fs", settings); startDeleteSnapshot(repoName, snapshot).get(); @@ -153,20 +146,12 @@ public void testConcurrentlyChangeRepositoryContentsInBwCMode() throws Exception Path repo = randomRepoPath(); final String repoName = "test-repo"; logger.info("--> creating repository at {}", repo.toAbsolutePath()); - assertAcked( - client.admin() - .cluster() - .preparePutRepository(repoName) - .setType("fs") - .setSettings( - Settings.builder() - .put("location", repo) - .put("compress", false) - .put(BlobStoreRepository.ALLOW_CONCURRENT_MODIFICATION.getKey(), true) - .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - ) - ); - + Settings.Builder settings = Settings.builder() + .put("location", repo) + .put("compress", false) + .put(BlobStoreRepository.ALLOW_CONCURRENT_MODIFICATION.getKey(), true) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES); + createRepository(repoName, "fs", settings); createIndex("test-idx-1", "test-idx-2"); logger.info("--> indexing some data"); indexRandom( diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java index 54db951eb41c2..79e3ea325503e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java @@ -86,6 +86,7 @@ import org.opensearch.rest.action.admin.cluster.RestGetRepositoriesAction; import org.opensearch.snapshots.mockstore.MockRepository; import org.opensearch.test.InternalTestCluster; +import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope; import org.opensearch.test.OpenSearchIntegTestCase.Scope; import org.opensearch.test.TestCustomMetadata; @@ -759,18 +760,24 @@ public void testRegistrationFailure() { internalCluster().startNode(nonClusterManagerNode()); // Register mock repositories for (int i = 0; i < 5; i++) { - clusterAdmin().preparePutRepository("test-repo" + i) - .setType("mock") - .setSettings(Settings.builder().put("location", randomRepoPath())) - .setVerify(false) - .get(); + OpenSearchIntegTestCase.putRepositoryRequestBuilder( + clusterAdmin(), + "test-repo" + i, + "mock", + false, + Settings.builder().put("location", randomRepoPath()), + null + ).get(); } logger.info("--> make sure that properly setup repository can be registered on all nodes"); - clusterAdmin().preparePutRepository("test-repo-0") - .setType("fs") - .setSettings(Settings.builder().put("location", randomRepoPath())) - .get(); - + OpenSearchIntegTestCase.putRepositoryRequestBuilder( + clusterAdmin(), + "test-repo-0", + "fs", + true, + Settings.builder().put("location", randomRepoPath()), + null + ).get(); } public void testThatSensitiveRepositorySettingsAreNotExposed() throws Exception { @@ -979,11 +986,7 @@ public void testSnapshotWithDateMath() { final String snapshotName = ""; logger.info("--> creating repository"); - assertAcked( - clusterAdmin().preparePutRepository(repo) - .setType("fs") - .setSettings(Settings.builder().put("location", randomRepoPath()).put("compress", randomBoolean())) - ); + createRepository(repo, "fs", Settings.builder().put("location", randomRepoPath()).put("compress", randomBoolean())); final String expression1 = nameExpressionResolver.resolveDateMathExpression(snapshotName); logger.info("--> creating date math snapshot"); diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java index 1c46e37dea93a..50a611a3df9c6 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java @@ -115,13 +115,14 @@ public void testConcurrentDeleteFromOtherCluster() throws InterruptedException { secondCluster.startClusterManagerOnlyNode(); secondCluster.startDataOnlyNode(); - secondCluster.client() - .admin() - .cluster() - .preparePutRepository(repoNameOnSecondCluster) - .setType("fs") - .setSettings(Settings.builder().put("location", repoPath)) - .get(); + OpenSearchIntegTestCase.putRepositoryRequestBuilder( + secondCluster.client().admin().cluster(), + repoNameOnSecondCluster, + "fs", + true, + Settings.builder().put("location", repoPath), + null + ).get(); createIndexWithRandomDocs("test-idx-1", randomIntBetween(1, 100)); createFullSnapshot(repoNameOnFirstCluster, "snap-1"); diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java index dd40c77ba918d..0d32290fadd86 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java @@ -56,7 +56,6 @@ import java.nio.file.Path; import java.util.List; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertRequestBuilderThrows; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -112,16 +111,7 @@ public void testRepositoryCreation() throws Exception { logger.info("--> check that trying to create a repository with the same settings repeatedly does not update cluster state"); String beforeStateUuid = clusterStateResponse.getState().stateUUID(); - assertThat( - client.admin() - .cluster() - .preparePutRepository("test-repo-1") - .setType("fs") - .setSettings(Settings.builder().put("location", location)) - .get() - .isAcknowledged(), - equalTo(true) - ); + createRepository("test-repo-1", "fs", Settings.builder().put("location", location)); assertEquals(beforeStateUuid, client.admin().cluster().prepareState().clear().get().getState().stateUUID()); logger.info("--> delete repository test-repo-1"); @@ -225,12 +215,7 @@ public void testMisconfiguredRepository() throws Exception { Path invalidRepoPath = createTempDir().toAbsolutePath(); String location = invalidRepoPath.toString(); try { - client().admin() - .cluster() - .preparePutRepository("test-repo") - .setType("fs") - .setSettings(Settings.builder().put("location", location)) - .get(); + createRepository("test-repo", "fs", Settings.builder().put("location", location)); fail("Shouldn't be here"); } catch (RepositoryException ex) { assertThat( @@ -242,33 +227,19 @@ public void testMisconfiguredRepository() throws Exception { public void testRepositoryAckTimeout() throws Exception { logger.info("--> creating repository test-repo-1 with 0s timeout - shouldn't ack"); - AcknowledgedResponse putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository("test-repo-1") - .setType("fs") - .setSettings( - Settings.builder() - .put("location", randomRepoPath()) - .put("compress", randomBoolean()) - .put("chunk_size", randomIntBetween(5, 100), ByteSizeUnit.BYTES) - ) - .setTimeout("0s") - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(false)); + + Settings.Builder settings = Settings.builder() + .put("location", randomRepoPath()) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(5, 100), ByteSizeUnit.BYTES); + createRepository("test-repo-1", "fs", settings, "0s"); logger.info("--> creating repository test-repo-2 with standard timeout - should ack"); - putRepositoryResponse = client().admin() - .cluster() - .preparePutRepository("test-repo-2") - .setType("fs") - .setSettings( - Settings.builder() - .put("location", randomRepoPath()) - .put("compress", randomBoolean()) - .put("chunk_size", randomIntBetween(5, 100), ByteSizeUnit.BYTES) - ) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + settings = Settings.builder() + .put("location", randomRepoPath()) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(5, 100), ByteSizeUnit.BYTES); + createRepository("test-repo-2", "fs", settings); logger.info("--> deleting repository test-repo-2 with 0s timeout - shouldn't ack"); AcknowledgedResponse deleteRepositoryResponse = client().admin() @@ -292,25 +263,43 @@ public void testRepositoryVerification() throws Exception { Settings readonlySettings = Settings.builder().put(settings).put("readonly", true).build(); logger.info("--> creating repository that cannot write any files - should fail"); assertRequestBuilderThrows( - client.admin().cluster().preparePutRepository("test-repo-1").setType("mock").setSettings(settings), + OpenSearchIntegTestCase.putRepositoryRequestBuilder( + client.admin().cluster(), + "test-repo-1", + "mock", + true, + Settings.builder().put(settings), + null + ), RepositoryVerificationException.class ); logger.info("--> creating read-only repository that cannot read any files - should fail"); assertRequestBuilderThrows( - client.admin().cluster().preparePutRepository("test-repo-2").setType("mock").setSettings(readonlySettings), + OpenSearchIntegTestCase.putRepositoryRequestBuilder( + client.admin().cluster(), + "test-repo-2", + "mock", + true, + Settings.builder().put(readonlySettings), + null + ), RepositoryVerificationException.class ); logger.info("--> creating repository that cannot write any files, but suppress verification - should be acked"); - assertAcked(client.admin().cluster().preparePutRepository("test-repo-1").setType("mock").setSettings(settings).setVerify(false)); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), "test-repo-1", "mock", false, Settings.builder().put(settings)); logger.info("--> verifying repository"); assertRequestBuilderThrows(client.admin().cluster().prepareVerifyRepository("test-repo-1"), RepositoryVerificationException.class); logger.info("--> creating read-only repository that cannot read any files, but suppress verification - should be acked"); - assertAcked( - client.admin().cluster().preparePutRepository("test-repo-2").setType("mock").setSettings(readonlySettings).setVerify(false) + OpenSearchIntegTestCase.putRepository( + client.admin().cluster(), + "test-repo-2", + "mock", + false, + Settings.builder().put(readonlySettings) ); logger.info("--> verifying repository"); @@ -320,12 +309,8 @@ public void testRepositoryVerification() throws Exception { logger.info("--> creating repository"); try { - client.admin() - .cluster() - .preparePutRepository("test-repo-1") - .setType("mock") - .setSettings(Settings.builder().put("location", location).put("localize_location", true)) - .get(); + Settings.Builder settingsBuilder = Settings.builder().put("location", location).put("localize_location", true); + createRepository("test-repo-1", "mock", settingsBuilder); fail("RepositoryVerificationException wasn't generated"); } catch (RepositoryVerificationException ex) { assertThat(ex.getMessage(), containsString("is not shared")); diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java index 0eb37703eb0f1..af87c7441cb75 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoryFilterUserMetadataIT.java @@ -58,7 +58,6 @@ import java.util.Map; import java.util.function.Function; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.hamcrest.Matchers.is; public class RepositoryFilterUserMetadataIT extends OpenSearchIntegTestCase { @@ -71,17 +70,10 @@ protected Collection> nodePlugins() { public void testFilteredRepoMetadataIsUsed() { final String clusterManagerName = internalCluster().getClusterManagerName(); final String repoName = "test-repo"; - assertAcked( - client().admin() - .cluster() - .preparePutRepository(repoName) - .setType(MetadataFilteringPlugin.TYPE) - .setSettings( - Settings.builder() - .put("location", randomRepoPath()) - .put(MetadataFilteringPlugin.CLUSTER_MANAGER_SETTING_VALUE, clusterManagerName) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", randomRepoPath()) + .put(MetadataFilteringPlugin.CLUSTER_MANAGER_SETTING_VALUE, clusterManagerName); + createRepository(repoName, MetadataFilteringPlugin.TYPE, settings); createIndex("test-idx"); final SnapshotInfo snapshotInfo = client().admin() .cluster() diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java index 91b0aa6438753..5c13fae2c660d 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java @@ -82,6 +82,7 @@ import org.opensearch.repositories.RepositoryException; import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.snapshots.mockstore.MockRepository; +import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.threadpool.ThreadPool; import java.nio.channels.SeekableByteChannel; @@ -389,17 +390,11 @@ public void testSnapshotFileFailureDuringSnapshot() throws InterruptedException disableRepoConsistencyCheck("This test uses a purposely broken repository so it would fail consistency checks"); logger.info("--> creating repository"); - assertAcked( - clusterAdmin().preparePutRepository("test-repo") - .setType("mock") - .setSettings( - Settings.builder() - .put("location", randomRepoPath()) - .put("random", randomAlphaOfLength(10)) - .put("random_control_io_exception_rate", 0.2) - ) - .setVerify(false) - ); + Settings.Builder settings = Settings.builder() + .put("location", randomRepoPath()) + .put("random", randomAlphaOfLength(10)) + .put("random_control_io_exception_rate", 0.2); + OpenSearchIntegTestCase.putRepository(clusterAdmin(), "test-repo", "mock", settings); createIndexWithRandomDocs("test-idx", 100); @@ -690,11 +685,8 @@ private void unrestorableUseCase( assertAcked(client().admin().indices().prepareDelete(indexName)); // update the test repository - assertAcked( - clusterAdmin().preparePutRepository("test-repo") - .setType("mock") - .setSettings(Settings.builder().put("location", repositoryLocation).put(repositorySettings).build()) - ); + Settings.Builder settings = Settings.builder().put("location", repositoryLocation).put(repositorySettings); + OpenSearchIntegTestCase.putRepository(clusterAdmin(), "test-repo", "mock", settings); // attempt to restore the snapshot with the given settings RestoreSnapshotResponse restoreResponse = clusterAdmin().prepareRestoreSnapshot("test-repo", "test-snap") @@ -1015,27 +1007,17 @@ public void testDeleteRepositoryWhileSnapshotting() throws Exception { } logger.info("--> trying to move repository to another location"); + Settings.Builder settings = Settings.builder().put("location", repositoryLocation.resolve("test")); try { - client.admin() - .cluster() - .preparePutRepository("test-repo") - .setType("fs") - .setSettings(Settings.builder().put("location", repositoryLocation.resolve("test"))) - .get(); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), "test-repo", "fs", settings); fail("shouldn't be able to replace in-use repository"); } catch (Exception ex) { logger.info("--> in-use repository replacement failed"); } logger.info("--> trying to create a repository with different name"); - assertAcked( - client.admin() - .cluster() - .preparePutRepository("test-repo-2") - .setVerify(false) // do not do verification itself as snapshot threads could be fully blocked - .setType("fs") - .setSettings(Settings.builder().put("location", repositoryLocation.resolve("test"))) - ); + Settings.Builder settingsBuilder = Settings.builder().put("location", repositoryLocation.resolve("test")); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), "test-repo-2", "fs", false, settingsBuilder); logger.info("--> unblocking blocked node"); unblockNode("test-repo", blockedNode); @@ -1941,20 +1923,12 @@ public void testSnapshotSucceedsAfterSnapshotFailure() throws Exception { logger.info("--> creating repository"); final Path repoPath = randomRepoPath(); final Client client = client(); - assertAcked( - client.admin() - .cluster() - .preparePutRepository("test-repo") - .setType("mock") - .setVerify(false) - .setSettings( - Settings.builder() - .put("location", repoPath) - .put("random_control_io_exception_rate", randomIntBetween(5, 20) / 100f) - // test that we can take a snapshot after a failed one, even if a partial index-N was written - .put("random", randomAlphaOfLength(10)) - ) - ); + Settings.Builder settings = Settings.builder() + .put("location", repoPath) + .put("random_control_io_exception_rate", randomIntBetween(5, 20) / 100f) + // test that we can take a snapshot after a failed one, even if a partial index-N was written + .put("random", randomAlphaOfLength(10)); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), "test-repo", "mock", false, settings); assertAcked( prepareCreate("test-idx").setSettings( @@ -2004,14 +1978,8 @@ public void testGetSnapshotsFromIndexBlobOnly() throws Exception { logger.info("--> creating repository"); final Path repoPath = randomRepoPath(); final Client client = client(); - assertAcked( - client.admin() - .cluster() - .preparePutRepository("test-repo") - .setType("fs") - .setVerify(false) - .setSettings(Settings.builder().put("location", repoPath)) - ); + Settings.Builder settings = Settings.builder().put("location", repoPath); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), "test-repo", "fs", false, settings); logger.info("--> creating random number of indices"); final int numIndices = randomIntBetween(1, 10); diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SystemRepositoryIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SystemRepositoryIT.java index 28b84655a2cc7..bb5cc89d4e1d5 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SystemRepositoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SystemRepositoryIT.java @@ -8,7 +8,6 @@ package org.opensearch.snapshots; -import org.opensearch.client.Client; import org.opensearch.common.settings.Settings; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.repositories.RepositoryException; @@ -19,7 +18,6 @@ import java.nio.file.Path; import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) public class SystemRepositoryIT extends AbstractSnapshotIntegTestCase { @@ -43,13 +41,8 @@ public void testRestrictedSettingsCantBeUpdated() { disableRepoConsistencyCheck("System repository is being used for the test"); internalCluster().startNode(); - final Client client = client(); final Settings.Builder repoSettings = Settings.builder().put("location", randomRepoPath()); - - RepositoryException e = expectThrows( - RepositoryException.class, - () -> client.admin().cluster().preparePutRepository(systemRepoName).setType("mock").setSettings(repoSettings).get() - ); + RepositoryException e = expectThrows(RepositoryException.class, () -> createRepository(systemRepoName, "mock", repoSettings)); assertEquals( e.getMessage(), "[system-repo-name] trying to modify an unmodifiable attribute type of system " @@ -59,18 +52,8 @@ public void testRestrictedSettingsCantBeUpdated() { public void testSystemRepositoryNonRestrictedSettingsCanBeUpdated() { disableRepoConsistencyCheck("System repository is being used for the test"); - internalCluster().startNode(); - final Client client = client(); final Settings.Builder repoSettings = Settings.builder().put("location", absolutePath).put("chunk_size", new ByteSizeValue(20)); - - assertAcked( - client.admin() - .cluster() - .preparePutRepository(systemRepoName) - .setType(ReloadableFsRepository.TYPE) - .setSettings(repoSettings) - .get() - ); + createRepository(systemRepoName, ReloadableFsRepository.TYPE, repoSettings); } } diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java index 29ffb94ce8bf4..958a499ada167 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryHelperTests.java @@ -9,7 +9,6 @@ package org.opensearch.repositories.blobstore; import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.service.ClusterService; @@ -99,25 +98,15 @@ protected void assertSnapshotOrGenericThread() { } protected void createRepository(Client client, String repoName) { - AcknowledgedResponse putRepositoryResponse = client.admin() - .cluster() - .preparePutRepository(repoName) - .setType(REPO_TYPE) - .setSettings( - Settings.builder().put(node().settings()).put("location", OpenSearchIntegTestCase.randomRepoPath(node().settings())) - ) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settings = Settings.builder() + .put(node().settings()) + .put("location", OpenSearchIntegTestCase.randomRepoPath(node().settings())); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), repoName, REPO_TYPE, settings); } protected void createRepository(Client client, String repoName, Settings repoSettings) { - AcknowledgedResponse putRepositoryResponse = client.admin() - .cluster() - .preparePutRepository(repoName) - .setType(REPO_TYPE) - .setSettings(repoSettings) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settingsBuilder = Settings.builder().put(repoSettings); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), repoName, REPO_TYPE, settingsBuilder); } protected void updateRepository(Client client, String repoName, Settings repoSettings) { diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java index 67c5f14a1c355..73d997a77e303 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -36,7 +36,6 @@ import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.opensearch.action.support.GroupedActionListener; import org.opensearch.action.support.PlainActionFuture; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.RepositoryMetadata; import org.opensearch.cluster.service.ClusterService; @@ -141,13 +140,8 @@ public void testRetrieveSnapshots() throws Exception { final String repositoryName = "test-repo"; logger.info("--> creating repository"); - AcknowledgedResponse putRepositoryResponse = client.admin() - .cluster() - .preparePutRepository(repositoryName) - .setType(REPO_TYPE) - .setSettings(Settings.builder().put(node().settings()).put("location", location)) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settings = Settings.builder().put(node().settings()).put("location", location); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), repositoryName, REPO_TYPE, settings); logger.info("--> creating an index and indexing documents"); final String indexName = "test-idx"; @@ -266,20 +260,13 @@ public void testBadChunksize() throws Exception { final Client client = client(); final Path location = OpenSearchIntegTestCase.randomRepoPath(node().settings()); final String repositoryName = "test-repo"; - + Settings.Builder settings = Settings.builder() + .put(node().settings()) + .put("location", location) + .put("chunk_size", randomLongBetween(-10, 0), ByteSizeUnit.BYTES); expectThrows( RepositoryException.class, - () -> client.admin() - .cluster() - .preparePutRepository(repositoryName) - .setType(REPO_TYPE) - .setSettings( - Settings.builder() - .put(node().settings()) - .put("location", location) - .put("chunk_size", randomLongBetween(-10, 0), ByteSizeUnit.BYTES) - ) - .get() + () -> OpenSearchIntegTestCase.putRepository(client.admin().cluster(), repositoryName, REPO_TYPE, settings) ); } @@ -287,18 +274,11 @@ public void testPrefixModeVerification() throws Exception { final Client client = client(); final Path location = OpenSearchIntegTestCase.randomRepoPath(node().settings()); final String repositoryName = "test-repo"; - AcknowledgedResponse putRepositoryResponse = client.admin() - .cluster() - .preparePutRepository(repositoryName) - .setType(REPO_TYPE) - .setSettings( - Settings.builder() - .put(node().settings()) - .put("location", location) - .put(BlobStoreRepository.PREFIX_MODE_VERIFICATION_SETTING.getKey(), true) - ) - .get(); - assertTrue(putRepositoryResponse.isAcknowledged()); + Settings.Builder settings = Settings.builder() + .put(node().settings()) + .put("location", location) + .put(BlobStoreRepository.PREFIX_MODE_VERIFICATION_SETTING.getKey(), true); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), repositoryName, REPO_TYPE, settings); final RepositoriesService repositoriesService = getInstanceFromNode(RepositoriesService.class); final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(repositoryName); @@ -332,13 +312,8 @@ private BlobStoreRepository setupRepo() { final Path location = OpenSearchIntegTestCase.randomRepoPath(node().settings()); final String repositoryName = "test-repo"; - AcknowledgedResponse putRepositoryResponse = client.admin() - .cluster() - .preparePutRepository(repositoryName) - .setType(REPO_TYPE) - .setSettings(Settings.builder().put(node().settings()).put("location", location)) - .get(); - assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true)); + Settings.Builder settings = Settings.builder().put(node().settings()).put("location", location); + OpenSearchIntegTestCase.putRepository(client.admin().cluster(), repositoryName, REPO_TYPE, settings); final RepositoriesService repositoriesService = getInstanceFromNode(RepositoriesService.class); final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(repositoryName); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 769dfeb37ff8d..299c11b0e1b28 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -229,6 +229,7 @@ import org.opensearch.tasks.TaskResourceTrackingService; import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; +import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.disruption.DisruptableMockTransport; import org.opensearch.threadpool.ThreadPool; @@ -1501,12 +1502,8 @@ private RepositoryData getRepositoryData(Repository repository) { private StepListener createRepoAndIndex(String repoName, String index, int shards) { final StepListener createRepositoryListener = new StepListener<>(); - client().admin() - .cluster() - .preparePutRepository(repoName) - .setType(FsRepository.TYPE) - .setSettings(Settings.builder().put("location", randomAlphaOfLength(10))) - .execute(createRepositoryListener); + Settings.Builder settings = Settings.builder().put("location", randomAlphaOfLength(10)); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), repoName, FsRepository.TYPE, settings, createRepositoryListener); final StepListener createIndexResponseStepListener = new StepListener<>(); diff --git a/test/framework/src/main/java/org/opensearch/repositories/blobstore/OpenSearchBlobStoreRepositoryIntegTestCase.java b/test/framework/src/main/java/org/opensearch/repositories/blobstore/OpenSearchBlobStoreRepositoryIntegTestCase.java index 789858ca38fad..4e0c9270cd627 100644 --- a/test/framework/src/main/java/org/opensearch/repositories/blobstore/OpenSearchBlobStoreRepositoryIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/repositories/blobstore/OpenSearchBlobStoreRepositoryIntegTestCase.java @@ -110,9 +110,7 @@ protected final String createRepository(final String name, final Settings settin final boolean verify = randomBoolean(); logger.debug("--> creating repository [name: {}, verify: {}, settings: {}]", name, verify, settings); - assertAcked( - client().admin().cluster().preparePutRepository(name).setType(repositoryType()).setVerify(verify).setSettings(settings) - ); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), name, repositoryType(), verify, Settings.builder().put(settings)); internalCluster().getDataOrClusterManagerNodeInstances(RepositoriesService.class).forEach(repositories -> { assertThat(repositories.repository(name), notNullValue()); diff --git a/test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java b/test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java index ce76914882150..8369a8c1f530e 100644 --- a/test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java @@ -103,7 +103,6 @@ import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.SEGMENTS; import static org.opensearch.index.remote.RemoteStoreEnums.DataType.LOCK_FILES; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; @@ -381,16 +380,6 @@ public void unblockNode(final String repository, final String node) { ((MockRepository) internalCluster().getInstance(RepositoriesService.class, node).repository(repository)).unblock(); } - protected void createRepository(String repoName, String type, Settings.Builder settings) { - logger.info("--> creating repository [{}] [{}]", repoName, type); - assertAcked(clusterAdmin().preparePutRepository(repoName).setType(type).setSettings(settings)); - } - - protected void updateRepository(String repoName, String type, Settings.Builder settings) { - logger.info("--> updating repository [{}] [{}]", repoName, type); - assertAcked(clusterAdmin().preparePutRepository(repoName).setType(type).setSettings(settings)); - } - protected void createRepository(String repoName, String type, Path location) { createRepository(repoName, type, Settings.builder().put("location", location)); } diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java index b86cce682c68e..023dc5fc5ff37 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java @@ -48,6 +48,7 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; import org.opensearch.action.admin.cluster.node.hotthreads.NodeHotThreads; import org.opensearch.action.admin.cluster.node.info.NodesInfoResponse; +import org.opensearch.action.admin.cluster.repositories.put.PutRepositoryRequestBuilder; import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; import org.opensearch.action.admin.cluster.state.ClusterStateResponse; import org.opensearch.action.admin.cluster.tasks.PendingClusterTasksResponse; @@ -70,6 +71,7 @@ import org.opensearch.action.search.SearchResponse; import org.opensearch.action.support.IndicesOptions; import org.opensearch.action.support.WriteRequest; +import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.AdminClient; import org.opensearch.client.Client; import org.opensearch.client.ClusterAdminClient; @@ -2555,6 +2557,73 @@ protected long getLatestSegmentInfoVersion(IndexShard shard) { } } + protected void createRepository(String repoName, String type, Settings.Builder settings, String timeout) { + logger.info("--> creating repository [{}] [{}]", repoName, type); + putRepository(clusterAdmin(), repoName, type, timeout, settings); + } + + protected void createRepository(String repoName, String type, Settings.Builder settings) { + logger.info("--> creating repository [{}] [{}]", repoName, type); + putRepository(clusterAdmin(), repoName, type, null, settings); + } + + protected void updateRepository(String repoName, String type, Settings.Builder settings) { + logger.info("--> updating repository [{}] [{}]", repoName, type); + putRepository(clusterAdmin(), repoName, type, null, settings); + } + + public static void putRepository(ClusterAdminClient adminClient, String repoName, String type, Settings.Builder settings) { + assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, null)); + } + + public static void putRepository( + ClusterAdminClient adminClient, + String repoName, + String type, + String timeout, + Settings.Builder settings + ) { + assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, timeout)); + } + + public static void putRepository( + ClusterAdminClient adminClient, + String repoName, + String type, + boolean verify, + Settings.Builder settings + ) { + assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, verify, settings, null)); + } + + public static void putRepository( + ClusterAdminClient adminClient, + String repoName, + String type, + Settings.Builder settings, + ActionListener listener + ) { + putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, null).execute(listener); + } + + public static PutRepositoryRequestBuilder putRepositoryRequestBuilder( + ClusterAdminClient adminClient, + String repoName, + String type, + boolean verify, + Settings.Builder settings, + String timeout + ) { + PutRepositoryRequestBuilder builder = adminClient.preparePutRepository(repoName) + .setType(type) + .setVerify(verify) + .setSettings(settings); + if (timeout != null) { + builder.setTimeout(timeout); + } + return builder; + } + public static Settings remoteStoreClusterSettings(String name, Path path) { return remoteStoreClusterSettings(name, path, name, path); } @@ -2794,5 +2863,4 @@ private static Settings buildRemoteStoreNodeAttributes( settings.put(RemoteStoreSettings.CLUSTER_REMOTE_STORE_TRANSLOG_METADATA.getKey(), randomBoolean()); return settings.build(); } - } From c02de6cce2afd914b6b2f0d30fd68567bbefb7d5 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Fri, 30 Aug 2024 01:03:42 +0530 Subject: [PATCH 07/12] Move the index shard path upload to sync upload Signed-off-by: Ashish Singh --- .../repositories/blobstore/BlobStoreRepository.java | 2 +- .../repositories/blobstore/ConfigBlobStoreFormat.java | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) 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 0d325fcb62d0f..376a814f35274 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -2175,7 +2175,7 @@ String writeIndexShardPaths(IndexId indexId, SnapshotId snapshotId, int shardCou PathType.fromCode(pathType), PathHashAlgorithm.fromCode(pathHashAlgorithm) ); - SNAPSHOT_SHARD_PATHS_FORMAT.writeAsyncWithUrgentPriority( + SNAPSHOT_SHARD_PATHS_FORMAT.write( shardPaths, snapshotShardPathBlobContainer(), blobName, diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java index 8127bf8c2a2a2..8803f4f965ea1 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java @@ -76,4 +76,9 @@ public void writeAsyncWithUrgentPriority(T obj, BlobContainer blobContainer, Str ((AsyncMultiStreamBlobContainer) blobContainer).asyncBlobUpload(remoteTransferContainer.createWriteContext(), listener); } } + + public void write(T obj, BlobContainer blobContainer, String name, ActionListener listener) throws IOException { + write(obj, blobContainer, name, new NoneCompressor(), ToXContent.EMPTY_PARAMS, XContentType.JSON, null, null); + listener.onResponse(null); + } } From 29ecb9a8e53544d676d343468038279689f1d37d Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sat, 31 Aug 2024 15:58:10 +0530 Subject: [PATCH 08/12] Fix spotless errors Signed-off-by: Ashish Singh --- .../azure/AzureStorageCleanupThirdPartyTests.java | 4 +--- .../repositories/gcs/GoogleCloudStorageThirdPartyTests.java | 2 -- .../org/opensearch/repositories/hdfs/HdfsRepositoryTests.java | 1 - .../test/java/org/opensearch/repositories/hdfs/HdfsTests.java | 1 - .../repositories/s3/S3RepositoryThirdPartyTests.java | 4 +--- .../repositories/s3/RepositoryCredentialsTests.java | 3 +-- 6 files changed, 3 insertions(+), 12 deletions(-) diff --git a/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java b/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java index 522940b6c7729..7f32f09602164 100644 --- a/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java +++ b/plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java @@ -38,7 +38,6 @@ import com.azure.storage.blob.models.BlobStorageException; import org.opensearch.action.ActionRunnable; import org.opensearch.action.support.PlainActionFuture; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.collect.Tuple; import org.opensearch.common.settings.MockSecureSettings; import org.opensearch.common.settings.SecureSettings; @@ -47,17 +46,16 @@ import org.opensearch.plugins.Plugin; import org.opensearch.repositories.AbstractThirdPartyRepositoryTestCase; import org.opensearch.repositories.blobstore.BlobStoreRepository; +import org.opensearch.test.OpenSearchIntegTestCase; import org.junit.AfterClass; import java.net.HttpURLConnection; import java.util.Collection; import java.util.function.Supplier; -import org.opensearch.test.OpenSearchIntegTestCase; import reactor.core.scheduler.Schedulers; import static org.hamcrest.Matchers.blankOrNullString; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; public class AzureStorageCleanupThirdPartyTests extends AbstractThirdPartyRepositoryTestCase { diff --git a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java index 727a923104338..860b30fdef9ca 100644 --- a/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java +++ b/plugins/repository-gcs/src/internalClusterTest/java/org/opensearch/repositories/gcs/GoogleCloudStorageThirdPartyTests.java @@ -32,7 +32,6 @@ package org.opensearch.repositories.gcs; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.settings.MockSecureSettings; import org.opensearch.common.settings.SecureSettings; import org.opensearch.common.settings.Settings; @@ -45,7 +44,6 @@ import java.util.Collection; import static org.hamcrest.Matchers.blankOrNullString; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; public class GoogleCloudStorageThirdPartyTests extends AbstractThirdPartyRepositoryTestCase { diff --git a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java index fd6f6ffb74741..60fdbea011a44 100644 --- a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java +++ b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsRepositoryTests.java @@ -34,7 +34,6 @@ import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters; import org.opensearch.action.admin.cluster.repositories.cleanup.CleanupRepositoryResponse; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.settings.MockSecureSettings; import org.opensearch.common.settings.SecureSettings; import org.opensearch.common.settings.Settings; diff --git a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java index 99cab6a197b41..130bbbf1d2198 100644 --- a/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java +++ b/plugins/repository-hdfs/src/test/java/org/opensearch/repositories/hdfs/HdfsTests.java @@ -35,7 +35,6 @@ import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.cluster.ClusterState; import org.opensearch.common.settings.Settings; diff --git a/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java b/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java index 4ba46c1632072..7db9a0d3ba790 100644 --- a/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java +++ b/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java @@ -31,10 +31,8 @@ package org.opensearch.repositories.s3; -import org.opensearch.test.OpenSearchIntegTestCase; import software.amazon.awssdk.services.s3.model.StorageClass; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.SuppressForbidden; import org.opensearch.common.blobstore.BlobMetadata; import org.opensearch.common.blobstore.BlobPath; @@ -44,6 +42,7 @@ import org.opensearch.plugins.Plugin; import org.opensearch.repositories.AbstractThirdPartyRepositoryTestCase; import org.opensearch.repositories.blobstore.BlobStoreRepository; +import org.opensearch.test.OpenSearchIntegTestCase; import org.junit.Before; import java.util.Collection; @@ -52,7 +51,6 @@ import java.util.concurrent.TimeUnit; import static org.hamcrest.Matchers.blankOrNullString; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; public class S3RepositoryThirdPartyTests extends AbstractThirdPartyRepositoryTestCase { diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java index c1b7d82dab6c3..21017160d77e5 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java @@ -32,7 +32,6 @@ package org.opensearch.repositories.s3; -import org.opensearch.test.OpenSearchIntegTestCase; import software.amazon.awssdk.auth.credentials.AwsCredentials; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.services.s3.DelegatingS3Client; @@ -56,6 +55,7 @@ import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; import org.opensearch.rest.action.admin.cluster.RestGetRepositoriesAction; +import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.OpenSearchSingleNodeTestCase; import org.opensearch.test.rest.FakeRestRequest; @@ -69,7 +69,6 @@ import static org.opensearch.repositories.s3.S3ClientSettings.ACCESS_KEY_SETTING; import static org.opensearch.repositories.s3.S3ClientSettings.SECRET_KEY_SETTING; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; From 596b6ded7e9be8b0f21624402b67079ecf789694 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sat, 31 Aug 2024 16:46:22 +0530 Subject: [PATCH 09/12] Do minor code refactor Signed-off-by: Ashish Singh --- .../blobstore/BlobStoreRepository.java | 15 ++------------- .../blobstore/ConfigBlobStoreFormat.java | 3 +-- 2 files changed, 3 insertions(+), 15 deletions(-) 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 49622224ab0a0..c8597e2de08af 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -2182,12 +2182,8 @@ String writeIndexShardPaths(IndexId indexId, SnapshotId snapshotId, int shardCou PathType.fromCode(pathType), PathHashAlgorithm.fromCode(pathHashAlgorithm) ); - SNAPSHOT_SHARD_PATHS_FORMAT.write( - shardPaths, - snapshotShardPathBlobContainer(), - blobName, - getSnapshotShardListener(indexId, snapshotId) - ); + SNAPSHOT_SHARD_PATHS_FORMAT.write(shardPaths, snapshotShardPathBlobContainer(), blobName); + logShardPathsOperationSuccess(indexId, snapshotId); return blobName; } catch (IOException e) { logShardPathsOperationWarning(indexId, snapshotId, e); @@ -2204,13 +2200,6 @@ private List getShardPaths(IndexId indexId, int shardCount) { return paths; } - private ActionListener getSnapshotShardListener(IndexId indexId, SnapshotId snapshotId) { - return ActionListener.wrap( - resp -> logShardPathsOperationSuccess(indexId, snapshotId), - ex -> logShardPathsOperationWarning(indexId, snapshotId, ex) - ); - } - private void logShardPathsOperationSuccess(IndexId indexId, SnapshotId snapshotId) { logger.trace( () -> new ParameterizedMessage( diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java index 8803f4f965ea1..630048c61785d 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/ConfigBlobStoreFormat.java @@ -77,8 +77,7 @@ public void writeAsyncWithUrgentPriority(T obj, BlobContainer blobContainer, Str } } - public void write(T obj, BlobContainer blobContainer, String name, ActionListener listener) throws IOException { + public void write(T obj, BlobContainer blobContainer, String name) throws IOException { write(obj, blobContainer, name, new NoneCompressor(), ToXContent.EMPTY_PARAMS, XContentType.JSON, null, null); - listener.onResponse(null); } } From 798cf4bc57c95209e652642689c9c6bb32b17aef Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sat, 31 Aug 2024 19:35:06 +0530 Subject: [PATCH 10/12] Fix tests Signed-off-by: Ashish Singh --- .../repositories/RepositoryBlocksIT.java | 3 +- .../RemoteStoreRepositoryRegistrationIT.java | 3 +- .../CorruptedBlobStoreRepositoryIT.java | 26 ++++++++++++---- .../DedicatedClusterSnapshotRestoreIT.java | 6 ++-- .../snapshots/MultiClusterRepoAccessIT.java | 3 +- .../opensearch/snapshots/RepositoriesIT.java | 6 ++-- .../blobstore/BlobStoreRepository.java | 8 ++--- .../snapshots/SnapshotShardPaths.java | 4 +-- .../blobstore/BlobStoreRepositoryTests.java | 2 +- .../test/OpenSearchIntegTestCase.java | 30 +++++++++++++------ 10 files changed, 62 insertions(+), 29 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java index dc1ace7c43e40..6c0a156eb6752 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/repositories/RepositoryBlocksIT.java @@ -63,7 +63,8 @@ public void testPutRepositoryWithBlocks() { "fs", false, settings, - null + null, + false ), Metadata.CLUSTER_READ_ONLY_BLOCK ); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java index 5e8761d837195..4cbafde6417af 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRepositoryRegistrationIT.java @@ -137,7 +137,8 @@ public void testMultiNodeClusterRandomNodeRecoverNetworkIsolationPostNonRestrict repositoryMetadata.type(), true, updatedSettings, - null + null, + false ).get(); ensureStableCluster(3, nodesInOneSide.stream().findAny().get()); diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java index a9d4242ec1bbf..95cc9eca92355 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/CorruptedBlobStoreRepositoryIT.java @@ -40,9 +40,13 @@ import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.metadata.RepositoriesMetadata; +import org.opensearch.common.blobstore.BlobPath; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.io.IOUtils; import org.opensearch.core.common.unit.ByteSizeUnit; +import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; +import org.opensearch.index.remote.RemoteStorePathStrategy; import org.opensearch.repositories.IndexId; import org.opensearch.repositories.RepositoriesService; import org.opensearch.repositories.Repository; @@ -58,6 +62,7 @@ import java.util.Map; import java.util.stream.Stream; +import static org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm.FNV_1A_COMPOSITE_1; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertRequestBuilderThrows; import static org.hamcrest.Matchers.containsString; @@ -358,9 +363,7 @@ public void testSnapshotWithCorruptedShardIndexFile() throws Exception { assertThat(indexIds.size(), equalTo(1)); final IndexId corruptedIndex = indexIds.get(indexName); - final Path shardIndexFile = repo.resolve("indices") - .resolve(corruptedIndex.getId()) - .resolve("0") + final Path shardIndexFile = repo.resolve(resolvePath(corruptedIndex, "0")) .resolve("index-" + repositoryData.shardGenerations().getShardGen(corruptedIndex, 0)); logger.info("--> truncating shard index file [{}]", shardIndexFile); @@ -435,7 +438,7 @@ public void testDeleteSnapshotWithMissingIndexAndShardMetadata() throws Exceptio logger.info("--> delete index metadata and shard metadata"); for (String index : indices) { - Path shardZero = indicesPath.resolve(indexIds.get(index).getId()).resolve("0"); + Path shardZero = repo.resolve(resolvePath(indexIds.get(index), "0")); if (randomBoolean()) { Files.delete( shardZero.resolve("index-" + getRepositoryData("test-repo").shardGenerations().getShardGen(indexIds.get(index), 0)) @@ -628,10 +631,9 @@ public void testSnapshotWithMissingShardLevelIndexFile() throws Exception { clusterAdmin().prepareCreateSnapshot("test-repo", "test-snap-1").setWaitForCompletion(true).setIndices("test-idx-*").get(); logger.info("--> deleting shard level index file"); - final Path indicesPath = repo.resolve("indices"); for (IndexId indexId : getRepositoryData("test-repo").getIndices().values()) { final Path shardGen; - try (Stream shardFiles = Files.list(indicesPath.resolve(indexId.getId()).resolve("0"))) { + try (Stream shardFiles = Files.list(repo.resolve(resolvePath(indexId, "0")))) { shardGen = shardFiles.filter(file -> file.getFileName().toString().startsWith(BlobStoreRepository.INDEX_FILE_PREFIX)) .findFirst() .orElseThrow(() -> new AssertionError("Failed to find shard index blob")); @@ -681,4 +683,16 @@ private void assertRepositoryBlocked(Client client, String repo, String existing containsString("Could not read repository data because the contents of the repository do not match its expected state.") ); } + + private static String resolvePath(IndexId indexId, String shardId) { + PathType pathType = PathType.fromCode(indexId.getShardPathType()); + RemoteStorePathStrategy.SnapshotShardPathInput shardPathInput = new RemoteStorePathStrategy.SnapshotShardPathInput.Builder() + .basePath(BlobPath.cleanPath()) + .indexUUID(indexId.getId()) + .shardId(shardId) + .build(); + PathHashAlgorithm pathHashAlgorithm = pathType != PathType.FIXED ? FNV_1A_COMPOSITE_1 : null; + BlobPath blobPath = pathType.path(shardPathInput, pathHashAlgorithm); + return blobPath.buildAsString(); + } } diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java index 79e3ea325503e..686853c42aa03 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/DedicatedClusterSnapshotRestoreIT.java @@ -766,7 +766,8 @@ public void testRegistrationFailure() { "mock", false, Settings.builder().put("location", randomRepoPath()), - null + null, + false ).get(); } logger.info("--> make sure that properly setup repository can be registered on all nodes"); @@ -776,7 +777,8 @@ public void testRegistrationFailure() { "fs", true, Settings.builder().put("location", randomRepoPath()), - null + null, + false ).get(); } diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java index 50a611a3df9c6..c96d4a2f079ee 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/MultiClusterRepoAccessIT.java @@ -121,7 +121,8 @@ public void testConcurrentDeleteFromOtherCluster() throws InterruptedException { "fs", true, Settings.builder().put("location", repoPath), - null + null, + false ).get(); createIndexWithRandomDocs("test-idx-1", randomIntBetween(1, 100)); diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java index 0d32290fadd86..2c399f59e4ee3 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java @@ -269,7 +269,8 @@ public void testRepositoryVerification() throws Exception { "mock", true, Settings.builder().put(settings), - null + null, + false ), RepositoryVerificationException.class ); @@ -282,7 +283,8 @@ public void testRepositoryVerification() throws Exception { "mock", true, Settings.builder().put(readonlySettings), - null + null, + false ), RepositoryVerificationException.class ); 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 c8597e2de08af..316c0eec8cae9 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -279,7 +279,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp public static final Setting SHARD_PATH_TYPE = new Setting<>( "shard_path_type", - PathType.HASHED_PREFIX.toString(), + PathType.FIXED.toString(), PathType::parseString ); @@ -1896,7 +1896,7 @@ private List findMatchingShardPaths(String indexId, Map findHighestGenerationShardPaths(List matchingShardPaths) { return matchingShardPaths.stream() - .map(s -> s.split(SnapshotShardPaths.DELIMITER)) + .map(s -> s.split("\\" + SnapshotShardPaths.DELIMITER)) .sorted((a, b) -> Integer.parseInt(b[2]) - Integer.parseInt(a[2])) .map(parts -> String.join(SnapshotShardPaths.DELIMITER, parts)) .findFirst(); @@ -2117,11 +2117,11 @@ public void finalizeSnapshot( */ private void cleanupRedundantSnapshotShardPaths(Set updatedShardPathsIndexIds) { Set updatedIndexIds = updatedShardPathsIndexIds.stream() - .map(s -> s.split(SnapshotShardPaths.DELIMITER)[0]) + .map(s -> s.split("\\" + SnapshotShardPaths.DELIMITER)[0]) .collect(Collectors.toSet()); Set indexIdShardPaths = getSnapshotShardPaths().keySet(); List staleShardPaths = indexIdShardPaths.stream().filter(s -> updatedShardPathsIndexIds.contains(s) == false).filter(s -> { - String indexId = s.split(SnapshotShardPaths.DELIMITER)[0]; + String indexId = s.split("\\" + SnapshotShardPaths.DELIMITER)[0]; return updatedIndexIds.contains(indexId); }).collect(Collectors.toList()); try { diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java index 40757b679c32a..88af14e2232f9 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java @@ -27,7 +27,7 @@ public class SnapshotShardPaths implements ToXContent { public static final String DIR = "snapshot_shard_paths"; - public static final String DELIMITER = "#"; + public static final String DELIMITER = "."; public static final String FILE_NAME_FORMAT = "%s"; @@ -96,7 +96,7 @@ public static SnapshotShardPaths fromXContent(XContentParser ignored) { * @throws IllegalArgumentException if the shard path format is invalid or cannot be parsed. */ public static ShardInfo parseShardPath(String shardPath) { - String[] parts = shardPath.split(SnapshotShardPaths.DELIMITER); + String[] parts = shardPath.split("\\" + SnapshotShardPaths.DELIMITER); if (parts.length != 5) { throw new IllegalArgumentException("Invalid shard path format: " + shardPath); } diff --git a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java index 8e03e231c7269..63257a5575970 100644 --- a/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java +++ b/server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java @@ -435,7 +435,7 @@ public void testWriteAndReadShardPaths() throws Exception { assertNotNull("IndexId should not be null", indexId); assertEquals("Index ID should match", shardInfo.getIndexId().getId(), indexId.getId()); assertEquals("Shard path type should match", shardInfo.getIndexId().getShardPathType(), indexId.getShardPathType()); - String[] parts = shardPathBlobName.split(SnapshotShardPaths.DELIMITER); + String[] parts = shardPathBlobName.split("\\" + SnapshotShardPaths.DELIMITER); assertEquals( "Path hash algorithm should be FNV_1A_COMPOSITE_1", RemoteStoreEnums.PathHashAlgorithm.FNV_1A_COMPOSITE_1, diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java index d56242a102f9f..de7dde5e9c6e9 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java @@ -2581,7 +2581,7 @@ protected void updateRepository(String repoName, String type, Settings.Builder s } public static void putRepository(ClusterAdminClient adminClient, String repoName, String type, Settings.Builder settings) { - assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, null)); + assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, null, false)); } public static void putRepository( @@ -2591,7 +2591,7 @@ public static void putRepository( String timeout, Settings.Builder settings ) { - assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, timeout)); + assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, timeout, false)); } public static void putRepository( @@ -2601,7 +2601,17 @@ public static void putRepository( boolean verify, Settings.Builder settings ) { - assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, verify, settings, null)); + assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, verify, settings, null, false)); + } + + public static void putRepositoryWithNoSettingOverrides( + ClusterAdminClient adminClient, + String repoName, + String type, + boolean verify, + Settings.Builder settings + ) { + assertAcked(putRepositoryRequestBuilder(adminClient, repoName, type, verify, settings, null, true)); } public static void putRepository( @@ -2611,7 +2621,7 @@ public static void putRepository( Settings.Builder settings, ActionListener listener ) { - putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, null).execute(listener); + putRepositoryRequestBuilder(adminClient, repoName, type, true, settings, null, false).execute(listener); } public static PutRepositoryRequestBuilder putRepositoryRequestBuilder( @@ -2620,15 +2630,17 @@ public static PutRepositoryRequestBuilder putRepositoryRequestBuilder( String type, boolean verify, Settings.Builder settings, - String timeout + String timeout, + boolean finalSettings ) { - PutRepositoryRequestBuilder builder = adminClient.preparePutRepository(repoName) - .setType(type) - .setVerify(verify) - .setSettings(settings); + PutRepositoryRequestBuilder builder = adminClient.preparePutRepository(repoName).setType(type).setVerify(verify); if (timeout != null) { builder.setTimeout(timeout); } + if (finalSettings == false) { + settings.put(BlobStoreRepository.SHARD_PATH_TYPE.getKey(), randomFrom(PathType.values())); + } + builder.setSettings(settings); return builder; } From 10f15ab73fb692021c683c141bd4175060285c81 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sat, 31 Aug 2024 22:14:27 +0530 Subject: [PATCH 11/12] More tests to use common methods for repo creation Signed-off-by: Ashish Singh --- .../remotestore/RemoteRestoreSnapshotIT.java | 215 +++++++----------- 1 file changed, 76 insertions(+), 139 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 42e44bd3f37c3..455a57071b287 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -803,20 +803,14 @@ public void testCreateSnapshotV2() throws Exception { Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - ); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true); + + createRepository(snapshotRepoName, FsRepository.TYPE, settings); Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); @@ -872,20 +866,14 @@ public void testMixedSnapshotCreationWithV2RepositorySetting() throws Exception Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), false) - ) - ); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), false); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); + Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); createIndex(indexName1, indexSettings); @@ -906,20 +894,14 @@ public void testMixedSnapshotCreationWithV2RepositorySetting() throws Exception assertThat(snapshotInfo.getPinnedTimestamp(), equalTo(0L)); // enable shallow_snapshot_v2 - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - ); + settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); + indexDocuments(client, indexName1, 10); indexDocuments(client, indexName2, 20); @@ -952,20 +934,13 @@ public void testConcurrentSnapshotV2CreateOperation() throws InterruptedExceptio Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - ); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); @@ -1034,20 +1009,13 @@ public void testCreateSnapshotV2WithRedIndex() throws Exception { Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - ); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); @@ -1088,20 +1056,13 @@ public void testCreateSnapshotV2WithIndexingLoad() throws Exception { Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - ); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); @@ -1162,20 +1123,13 @@ public void testCreateSnapshotV2WithShallowCopySettingDisabled() throws Exceptio Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), false) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - ); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), false) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); @@ -1217,20 +1171,13 @@ public void testClusterManagerFailoverDuringSnapshotCreation() throws Exception Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - ); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); @@ -1295,21 +1242,14 @@ public void testConcurrentV1SnapshotAndV2RepoSettingUpdate() throws Exception { String snapshotName1 = "test-create-snapshot-v1"; Path absolutePath1 = randomRepoPath().toAbsolutePath(); logger.info("Snapshot Path [{}]", absolutePath1); + Settings.Builder settings = Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), false); + createRepository(snapshotRepoName, FsRepository.TYPE, settings); - assertAcked( - client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), false) - ) - ); Client client = client(); Settings indexSettings = getIndexSettings(20, 0).build(); @@ -1346,19 +1286,16 @@ public void testConcurrentV1SnapshotAndV2RepoSettingUpdate() throws Exception { assertThrows( IllegalStateException.class, - () -> client().admin() - .cluster() - .preparePutRepository(snapshotRepoName) - .setType(FsRepository.TYPE) - .setSettings( - Settings.builder() - .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) - .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) - .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) - .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) - .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) - ) - .get() + () -> createRepository( + snapshotRepoName, + FsRepository.TYPE, + Settings.builder() + .put(FsRepository.LOCATION_SETTING.getKey(), absolutePath1) + .put(FsRepository.COMPRESS_SETTING.getKey(), randomBoolean()) + .put(FsRepository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(100, 1000), ByteSizeUnit.BYTES) + .put(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY.getKey(), true) + .put(BlobStoreRepository.SHALLOW_SNAPSHOT_V2.getKey(), true) + ) ); } catch (Exception e) { From 0dea663c8958f2b38b15da545e2ac26d46267e7c Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sun, 1 Sep 2024 18:58:43 +0530 Subject: [PATCH 12/12] Address comments and fix multiple failing tests Signed-off-by: Ashish Singh --- .../RemoteStoreClusterStateRestoreIT.java | 12 ++++++++++-- .../opensearch/snapshots/RepositoriesIT.java | 12 +++++++++++- .../SharedClusterSnapshotRestoreIT.java | 2 +- .../common/settings/IndexScopedSettings.java | 1 + .../org/opensearch/index/IndexSettings.java | 9 +++++++++ .../RemoteSnapshotDirectoryFactory.java | 19 +++++++++++++------ .../blobstore/BlobStoreRepository.java | 7 +++++-- .../opensearch/snapshots/RestoreService.java | 2 ++ 8 files changed, 52 insertions(+), 12 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java index c165e5c6727d6..6ec973090883b 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreClusterStateRestoreIT.java @@ -36,6 +36,8 @@ import org.opensearch.gateway.remote.ClusterMetadataManifest; import org.opensearch.gateway.remote.ClusterMetadataManifest.UploadedIndexMetadata; import org.opensearch.gateway.remote.RemoteClusterStateService; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; +import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.test.InternalTestCluster; import org.opensearch.test.OpenSearchIntegTestCase; import org.junit.Before; @@ -486,9 +488,15 @@ private void verifyRestoredRepositories(Path repoPath) { assertTrue(SYSTEM_REPOSITORY_SETTING.get(repositoriesMetadata.repository(REPOSITORY_NAME).settings())); assertTrue(SYSTEM_REPOSITORY_SETTING.get(repositoriesMetadata.repository(REPOSITORY_2_NAME).settings())); assertEquals("fs", repositoriesMetadata.repository("custom-repo").type()); + Settings settings = repositoriesMetadata.repository("custom-repo").settings(); + PathType pathType = BlobStoreRepository.SHARD_PATH_TYPE.get(settings); assertEquals( - Settings.builder().put("location", repoPath).put("compress", false).build(), - repositoriesMetadata.repository("custom-repo").settings() + Settings.builder() + .put("location", repoPath) + .put("compress", false) + .put(BlobStoreRepository.SHARD_PATH_TYPE.getKey(), pathType) + .build(), + settings ); // repo cleanup post verification diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java index 2c399f59e4ee3..97c09fc379bc8 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/RepositoriesIT.java @@ -33,6 +33,7 @@ package org.opensearch.snapshots; import org.opensearch.action.admin.cluster.repositories.get.GetRepositoriesResponse; +import org.opensearch.action.admin.cluster.repositories.put.PutRepositoryRequestBuilder; import org.opensearch.action.admin.cluster.repositories.verify.VerifyRepositoryResponse; import org.opensearch.action.admin.cluster.state.ClusterStateResponse; import org.opensearch.action.bulk.BulkRequest; @@ -232,7 +233,16 @@ public void testRepositoryAckTimeout() throws Exception { .put("location", randomRepoPath()) .put("compress", randomBoolean()) .put("chunk_size", randomIntBetween(5, 100), ByteSizeUnit.BYTES); - createRepository("test-repo-1", "fs", settings, "0s"); + PutRepositoryRequestBuilder requestBuilder = OpenSearchIntegTestCase.putRepositoryRequestBuilder( + client().admin().cluster(), + "test-repo-1", + "fs", + true, + settings, + "0s", + false + ); + assertFalse(requestBuilder.get().isAcknowledged()); logger.info("--> creating repository test-repo-2 with standard timeout - should ack"); settings = Settings.builder() diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java index 5c13fae2c660d..d2003f5dca0e2 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SharedClusterSnapshotRestoreIT.java @@ -394,7 +394,7 @@ public void testSnapshotFileFailureDuringSnapshot() throws InterruptedException .put("location", randomRepoPath()) .put("random", randomAlphaOfLength(10)) .put("random_control_io_exception_rate", 0.2); - OpenSearchIntegTestCase.putRepository(clusterAdmin(), "test-repo", "mock", settings); + OpenSearchIntegTestCase.putRepository(clusterAdmin(), "test-repo", "mock", false, settings); createIndexWithRandomDocs("test-idx", 100); diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 4d56c93d278da..9cad80e8e3e82 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -223,6 +223,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID, IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME, IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID, + IndexSettings.SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE, // Settings for remote translog IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING, diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 0811de770d65a..12715b353c526 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -48,6 +48,7 @@ import org.opensearch.core.common.unit.ByteSizeUnit; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.index.Index; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.index.remote.RemoteStorePathStrategy; import org.opensearch.index.remote.RemoteStoreUtils; import org.opensearch.index.translog.Translog; @@ -677,6 +678,14 @@ public static IndexMergePolicy fromString(String text) { Property.InternalIndex ); + public static final Setting SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE = new Setting<>( + "index.searchable_snapshot.shard_path_type", + PathType.FIXED.toString(), + PathType::parseString, + Property.IndexScope, + Property.InternalIndex + ); + public static final Setting DEFAULT_SEARCH_PIPELINE = new Setting<>( "index.search.default_pipeline", SearchPipelineService.NOOP_PIPELINE_ID, diff --git a/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java index 177f0526e7571..e027e8b7cb3b1 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java @@ -11,14 +11,15 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.store.FSDirectory; import org.opensearch.common.blobstore.BlobContainer; -import org.opensearch.common.blobstore.BlobPath; import org.opensearch.index.IndexSettings; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.index.shard.ShardPath; import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot; import org.opensearch.index.snapshots.blobstore.IndexShardSnapshot; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.utils.TransferManager; import org.opensearch.plugins.IndexStorePlugin; +import org.opensearch.repositories.IndexId; import org.opensearch.repositories.RepositoriesService; import org.opensearch.repositories.Repository; import org.opensearch.repositories.blobstore.BlobStoreRepository; @@ -74,10 +75,11 @@ private Future createRemoteSnapshotDirectoryFromSnapsho ShardPath localShardPath, BlobStoreRepository blobStoreRepository ) throws IOException { - final BlobPath blobPath = blobStoreRepository.basePath() - .add("indices") - .add(IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.get(indexSettings.getSettings())) - .add(Integer.toString(localShardPath.getShardId().getId())); + // The below information like the snapshot generated indexId, shard_path_type and shardId are used for + // creating the shard BlobContainer. This information has been updated as per the hashed_prefix snapshots. + String indexId = IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.get(indexSettings.getSettings()); + PathType pathType = IndexSettings.SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE.get(indexSettings.getSettings()); + int shardId = localShardPath.getShardId().getId(); final SnapshotId snapshotId = new SnapshotId( IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.get(indexSettings.getSettings()), IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.get(indexSettings.getSettings()) @@ -89,7 +91,12 @@ private Future createRemoteSnapshotDirectoryFromSnapsho // this trick is needed to bypass assertions in BlobStoreRepository::assertAllowableThreadPools in case of node restart and a remote // index restore is invoked return threadPool.executor(ThreadPool.Names.SNAPSHOT).submit(() -> { - final BlobContainer blobContainer = blobStoreRepository.blobStore().blobContainer(blobPath); + // shardContainer(IndexId, shardId) method uses the id and pathType information to generate the blobPath and + // hence the blobContainer. We have used a dummy name as it plays no relevance in the blobPath generation. + final BlobContainer blobContainer = blobStoreRepository.shardContainer( + new IndexId("DUMMY", indexId, pathType.getCode()), + shardId + ); final IndexShardSnapshot indexShardSnapshot = blobStoreRepository.loadShardSnapshot(blobContainer, snapshotId); assert indexShardSnapshot instanceof BlobStoreIndexShardSnapshot : "indexShardSnapshot should be an instance of BlobStoreIndexShardSnapshot"; 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 316c0eec8cae9..d2df42fed6db5 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -1833,14 +1833,17 @@ private void executeOneStaleIndexDelete( logger.debug("[{}] Found stale index [{}]. Cleaning it up", metadata.name(), indexSnId); List matchingShardPaths = findMatchingShardPaths(indexSnId, snapshotShardPaths); Optional highestGenShardPaths = findHighestGenerationShardPaths(matchingShardPaths); + + // The shardInfo can be null for 1) snapshots that pre-dates the hashed prefix snapshots. + // 2) Snapshot shard paths file upload failed + // In such cases, we fallback to fixed_path for cleanup of the data. ShardInfo shardInfo = getShardInfo(highestGenShardPaths, idToShardInfoMap, indexSnId); if (remoteStoreLockManagerFactory != null) { cleanupRemoteStoreLocks(indexEntry, shardInfo, remoteStoreLockManagerFactory); } - // The below deleteShardData would be no-op for FIXED path type snapshots. Otherwise, this deletes the - // shard level data for the underlying index based on the pathType present in IndexId + // Deletes the shard level data for the underlying index based on the shardInfo that was obtained above. DeleteResult deleteResult = deleteShardData(shardInfo); // If there are matchingShardPaths, then we delete them after we have deleted the shard data. diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index e464b71debf81..174cad50b00e2 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -86,6 +86,7 @@ import org.opensearch.core.index.shard.ShardId; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.index.shard.IndexShard; import org.opensearch.index.snapshots.IndexShardSnapshotStatus; import org.opensearch.index.store.remote.filecache.FileCacheStats; @@ -1328,6 +1329,7 @@ private static IndexMetadata addSnapshotToIndexSettings(IndexMetadata metadata, .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_INDEX_ID.getKey(), indexId.getId()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_SHARD_PATH_TYPE.getKey(), PathType.fromCode(indexId.getShardPathType())) .build(); return IndexMetadata.builder(metadata).settings(newSettings).build(); }