Skip to content

Commit a1006cc

Browse files
Fix assertion failure while deleting remote backed index (#14601)
Signed-off-by: Sachin Kale <[email protected]> (cherry picked from commit 74230b7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 3a1be63 commit a1006cc

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
@@ -597,7 +597,21 @@ public synchronized IndexShard createShard(
597597
this.indexSettings.getRemoteStorePathStrategy()
598598
);
599599
}
600-
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, lock, Store.OnClose.EMPTY, path);
600+
// When an instance of Store is created, a shardlock is created which is released on closing the instance of store.
601+
// Currently, we create 2 instances of store for remote store backed indices: store and remoteStore.
602+
// As there can be only one shardlock acquired for a given shard, the lock is shared between store and remoteStore.
603+
// This creates an issue when we are deleting the index as it results in closing both store and remoteStore.
604+
// Sample test failure: https://github.com/opensearch-project/OpenSearch/issues/13871
605+
// The following method provides ShardLock that is not maintained by NodeEnvironment.
606+
// As part of https://github.com/opensearch-project/OpenSearch/issues/13075, we want to move away from keeping 2
607+
// store instances.
608+
ShardLock remoteStoreLock = new ShardLock(shardId) {
609+
@Override
610+
protected void closeInternal() {
611+
// Do nothing for shard lock on remote store
612+
}
613+
};
614+
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, remoteStoreLock, Store.OnClose.EMPTY, path);
601615
} else {
602616
// Disallow shards with remote store based settings to be created on non-remote store enabled nodes
603617
// Even though we have `RemoteStoreMigrationAllocationDecider` in place to prevent something like this from happening at the
@@ -620,7 +634,6 @@ public synchronized IndexShard createShard(
620634
} else {
621635
directory = directoryFactory.newDirectory(this.indexSettings, path);
622636
}
623-
624637
store = new Store(
625638
shardId,
626639
this.indexSettings,

0 commit comments

Comments
 (0)