Skip to content

Commit

Permalink
Fix assertion failure while deleting remote backed index
Browse files Browse the repository at this point in the history
Signed-off-by: Sachin Kale <[email protected]>
  • Loading branch information
Sachin Kale committed Jun 30, 2024
1 parent 243e8db commit 5a942ff
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 4 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,19 @@ private void testPeerRecovery(int numberOfIterations, boolean invokeFlush) throw
);
}

public void testRemoteStoreIndexCreationAndDeletion() 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);

indexShard.store().incRef();
assertAcked(client().admin().indices().prepareDelete(INDEX_NAME));
indexShard.store().decRef();
}

public void testPeerRecoveryWithRemoteStoreAndRemoteTranslogNoDataFlush() throws Exception {
testPeerRecovery(1, true);
}
Expand Down
26 changes: 25 additions & 1 deletion server/src/main/java/org/opensearch/index/IndexService.java
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,14 @@ public synchronized IndexShard createShard(
this.indexSettings.getRemoteStorePathStrategy()
);
}
remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, lock, Store.OnClose.EMPTY, path);
remoteStore = new Store(
shardId,
this.indexSettings,
remoteDirectory,
getRemoteStoreLock(shardId),
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 Down Expand Up @@ -678,6 +685,23 @@ public synchronized IndexShard createShard(
}
}

// 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.
private ShardLock getRemoteStoreLock(ShardId shardId) {
return new ShardLock(shardId) {
@Override
protected void closeInternal() {
// Ignore for remote store
}
};
}

/*
Fetches the shard path based on the index type -
For a remote snapshot index, the cache path is used to initialize the shards.
Expand Down

0 comments on commit 5a942ff

Please sign in to comment.