Skip to content

Commit 74230b7

Browse files
authored
Fix assertion failure while deleting remote backed index (#14601)
Signed-off-by: Sachin Kale <kalsac@amazon.com>
1 parent 58d1164 commit 74230b7

File tree

3 files changed

+30
-5
lines changed

3 files changed

+30
-5
lines changed

server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteMigrationIndexMetadataUpdateIT.java

-2
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,6 @@ initalMetadataVersion < internalCluster().client()
275275
* After shard relocation completes, shuts down the docrep nodes and asserts remote
276276
* index settings are applied even when the index is in YELLOW state
277277
*/
278-
@AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/13737")
279278
public void testIndexSettingsUpdatedEvenForMisconfiguredReplicas() throws Exception {
280279
internalCluster().startClusterManagerOnlyNode();
281280

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

server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java

+15-1
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@
6565
import static org.opensearch.index.remote.RemoteStoreEnums.DataType.METADATA;
6666
import static org.opensearch.index.shard.IndexShardTestCase.getTranslog;
6767
import static org.opensearch.indices.RemoteStoreSettings.CLUSTER_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING;
68-
import static org.opensearch.test.OpenSearchTestCase.getShardLevelBlobPath;
6968
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
7069
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount;
7170
import static org.hamcrest.Matchers.comparesEqualTo;
@@ -133,6 +132,21 @@ private void testPeerRecovery(int numberOfIterations, boolean invokeFlush) throw
133132
);
134133
}
135134

135+
public void testRemoteStoreIndexCreationAndDeletionWithReferencedStore() throws InterruptedException, ExecutionException {
136+
String dataNode = internalCluster().startNodes(1).get(0);
137+
createIndex(INDEX_NAME, remoteStoreIndexSettings(0));
138+
ensureYellowAndNoInitializingShards(INDEX_NAME);
139+
ensureGreen(INDEX_NAME);
140+
141+
IndexShard indexShard = getIndexShard(dataNode, INDEX_NAME);
142+
143+
// Simulating a condition where store is already in use by increasing ref count, this helps in testing index
144+
// deletion when refresh is in-progress.
145+
indexShard.store().incRef();
146+
assertAcked(client().admin().indices().prepareDelete(INDEX_NAME));
147+
indexShard.store().decRef();
148+
}
149+
136150
public void testPeerRecoveryWithRemoteStoreAndRemoteTranslogNoDataFlush() throws Exception {
137151
testPeerRecovery(1, true);
138152
}

server/src/main/java/org/opensearch/index/IndexService.java

+15-2
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,21 @@ public synchronized IndexShard createShard(
602602
this.indexSettings.getRemoteStorePathStrategy()
603603
);
604604
}
605-
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, lock, Store.OnClose.EMPTY, path);
605+
// When an instance of Store is created, a shardlock is created which is released on closing the instance of store.
606+
// Currently, we create 2 instances of store for remote store backed indices: store and remoteStore.
607+
// As there can be only one shardlock acquired for a given shard, the lock is shared between store and remoteStore.
608+
// This creates an issue when we are deleting the index as it results in closing both store and remoteStore.
609+
// Sample test failure: https://github.com/opensearch-project/OpenSearch/issues/13871
610+
// The following method provides ShardLock that is not maintained by NodeEnvironment.
611+
// As part of https://github.com/opensearch-project/OpenSearch/issues/13075, we want to move away from keeping 2
612+
// store instances.
613+
ShardLock remoteStoreLock = new ShardLock(shardId) {
614+
@Override
615+
protected void closeInternal() {
616+
// Do nothing for shard lock on remote store
617+
}
618+
};
619+
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, remoteStoreLock, Store.OnClose.EMPTY, path);
606620
} else {
607621
// Disallow shards with remote store based settings to be created on non-remote store enabled nodes
608622
// Even though we have `RemoteStoreMigrationAllocationDecider` in place to prevent something like this from happening at the
@@ -625,7 +639,6 @@ public synchronized IndexShard createShard(
625639
} else {
626640
directory = directoryFactory.newDirectory(this.indexSettings, path);
627641
}
628-
629642
store = new Store(
630643
shardId,
631644
this.indexSettings,

0 commit comments

Comments
 (0)