Skip to content

Commit

Permalink
Fix assertion failure while deleting remote backed index (opensearch-…
Browse files Browse the repository at this point in the history
…project#14601)

Signed-off-by: Sachin Kale <kalsac@amazon.com>
  • Loading branch information
sachinpkale authored and akolarkunnu committed Jul 9, 2024
1 parent 7ea5aaa commit f9092f6
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,6 @@ initalMetadataVersion < internalCluster().client()
* After shard relocation completes, shuts down the docrep nodes and asserts remote
* index settings are applied even when the index is in YELLOW state
*/
@AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/13737")
public void testIndexSettingsUpdatedEvenForMisconfiguredReplicas() throws Exception {
internalCluster().startClusterManagerOnlyNode();

Expand Down Expand Up @@ -332,7 +331,6 @@ public void testIndexSettingsUpdatedEvenForMisconfiguredReplicas() throws Except
* After shard relocation completes, restarts the docrep node holding extra replica shard copy
* and asserts remote index settings are applied as soon as the docrep replica copy is unassigned
*/
@AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/13871")
public void testIndexSettingsUpdatedWhenDocrepNodeIsRestarted() throws Exception {
internalCluster().startClusterManagerOnlyNode();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
import static org.opensearch.index.remote.RemoteStoreEnums.DataType.METADATA;
import static org.opensearch.index.shard.IndexShardTestCase.getTranslog;
import static org.opensearch.indices.RemoteStoreSettings.CLUSTER_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING;
import static org.opensearch.test.OpenSearchTestCase.getShardLevelBlobPath;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount;
import static org.hamcrest.Matchers.comparesEqualTo;
Expand Down Expand Up @@ -133,6 +132,21 @@ private void testPeerRecovery(int numberOfIterations, boolean invokeFlush) throw
);
}

public void testRemoteStoreIndexCreationAndDeletionWithReferencedStore() throws InterruptedException, ExecutionException {
String dataNode = internalCluster().startNodes(1).get(0);
createIndex(INDEX_NAME, remoteStoreIndexSettings(0));
ensureYellowAndNoInitializingShards(INDEX_NAME);
ensureGreen(INDEX_NAME);

IndexShard indexShard = getIndexShard(dataNode, INDEX_NAME);

// Simulating a condition where store is already in use by increasing ref count, this helps in testing index
// deletion when refresh is in-progress.
indexShard.store().incRef();
assertAcked(client().admin().indices().prepareDelete(INDEX_NAME));
indexShard.store().decRef();
}

public void testPeerRecoveryWithRemoteStoreAndRemoteTranslogNoDataFlush() throws Exception {
testPeerRecovery(1, true);
}
Expand Down
17 changes: 15 additions & 2 deletions server/src/main/java/org/opensearch/index/IndexService.java
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,21 @@ public synchronized IndexShard createShard(
this.indexSettings.getRemoteStorePathStrategy()
);
}
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, lock, Store.OnClose.EMPTY, path);
// When an instance of Store is created, a shardlock is created which is released on closing the instance of store.
// Currently, we create 2 instances of store for remote store backed indices: store and remoteStore.
// As there can be only one shardlock acquired for a given shard, the lock is shared between store and remoteStore.
// This creates an issue when we are deleting the index as it results in closing both store and remoteStore.
// Sample test failure: https://github.com/opensearch-project/OpenSearch/issues/13871
// The following method provides ShardLock that is not maintained by NodeEnvironment.
// As part of https://github.com/opensearch-project/OpenSearch/issues/13075, we want to move away from keeping 2
// store instances.
ShardLock remoteStoreLock = new ShardLock(shardId) {
@Override
protected void closeInternal() {
// Do nothing for shard lock on remote store
}
};
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, remoteStoreLock, Store.OnClose.EMPTY, path);
} else {
// Disallow shards with remote store based settings to be created on non-remote store enabled nodes
// Even though we have `RemoteStoreMigrationAllocationDecider` in place to prevent something like this from happening at the
Expand All @@ -625,7 +639,6 @@ public synchronized IndexShard createShard(
} else {
directory = directoryFactory.newDirectory(this.indexSettings, path);
}

store = new Store(
shardId,
this.indexSettings,
Expand Down

0 comments on commit f9092f6

Please sign in to comment.