Skip to content

Commit 5a942ff

Browse files
author
Sachin Kale
committed
Fix assertion failure while deleting remote backed index
Signed-off-by: Sachin Kale <kalsac@amazon.com>
1 parent 243e8db commit 5a942ff

File tree

3 files changed

+38
-4
lines changed

3 files changed

+38
-4
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

+13-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,19 @@ private void testPeerRecovery(int numberOfIterations, boolean invokeFlush) throw
133132
);
134133
}
135134

135+
public void testRemoteStoreIndexCreationAndDeletion() 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+
indexShard.store().incRef();
144+
assertAcked(client().admin().indices().prepareDelete(INDEX_NAME));
145+
indexShard.store().decRef();
146+
}
147+
136148
public void testPeerRecoveryWithRemoteStoreAndRemoteTranslogNoDataFlush() throws Exception {
137149
testPeerRecovery(1, true);
138150
}

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

+25-1
Original file line numberDiff line numberDiff line change
@@ -597,7 +597,14 @@ public synchronized IndexShard createShard(
597597
this.indexSettings.getRemoteStorePathStrategy()
598598
);
599599
}
600-
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, lock, Store.OnClose.EMPTY, path);
600+
remoteStore = new Store(
601+
shardId,
602+
this.indexSettings,
603+
remoteDirectory,
604+
getRemoteStoreLock(shardId),
605+
Store.OnClose.EMPTY,
606+
path
607+
);
601608
} else {
602609
// Disallow shards with remote store based settings to be created on non-remote store enabled nodes
603610
// Even though we have `RemoteStoreMigrationAllocationDecider` in place to prevent something like this from happening at the
@@ -678,6 +685,23 @@ public synchronized IndexShard createShard(
678685
}
679686
}
680687

688+
// When an instance of Store is created, a shardlock is created which is released on closing the instance of store.
689+
// Currently, we create 2 instances of store for remote store backed indices: store and remoteStore.
690+
// As there can be only one shardlock acquired for a given shard, the lock is shared between store and remoteStore.
691+
// This creates an issue when we are deleting the index as it results in closing both store and remoteStore.
692+
// Sample test failure: https://github.com/opensearch-project/OpenSearch/issues/13871
693+
// The following method provides ShardLock that is not maintained by NodeEnvironment.
694+
// As part of https://github.com/opensearch-project/OpenSearch/issues/13075, we want to move away from keeping 2
695+
// store instances.
696+
private ShardLock getRemoteStoreLock(ShardId shardId) {
697+
return new ShardLock(shardId) {
698+
@Override
699+
protected void closeInternal() {
700+
// Ignore for remote store
701+
}
702+
};
703+
}
704+
681705
/*
682706
Fetches the shard path based on the index type -
683707
For a remote snapshot index, the cache path is used to initialize the shards.

0 commit comments

Comments
 (0)