Async close of `IndexShard` #108145

DaveCTurner · 2024-05-01T14:27:15Z

Moves the work to close an IndexShard, including any final flush and
waiting for merges to complete, off the cluster applier thread to avoid
delaying the application of cluster state updates.

Relates #89821
Relates #107513
Relates #108096
Relates ES-8334

Moves the work to close an `IndexShard`, including any final flush and waiting for merges to complete, off the cluster applier thread to avoid delaying the application of cluster state updates. Relates elastic#89821 Relates elastic#107513 Relates elastic#108096 Relates ES-8334

elasticsearchmachine · 2024-05-01T14:27:38Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticsearchmachine · 2024-05-01T14:27:38Z

Hi @DaveCTurner, I've created a changelog YAML for you.

henningandersen

Looks great. I wonder if we can add an integration test to validate that closing shards no longer blocks the applier thread?

henningandersen · 2024-05-01T15:11:59Z

server/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java

+     */
+    public void onClusterStateShardsClosed(Runnable action) {
+        // In practice, must be called from the applier thread and we should validate the last-applied cluster state version too
+        // ES-8334 TODO validate this is called properly


Is this a todo to be solved in this PR?

I've left a few items like this on the JIRA ticket, not intending to solve them here but will follow up in due course.

henningandersen · 2024-05-01T16:20:08Z

test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java

@@ -460,4 +463,11 @@ protected void ensureNoInitializingShards() {
    protected boolean enableConcurrentSearch() {
        return true;
    }
+
+    protected void awaitIndexShardCloseAsyncTasks() {
+        // ES-8334 TODO build this wait into the relevant APIs (especially, delete-index and close-index)


I wonder about this, it seems that responding early is fine, given that for instance delete-index still does not guarantee files being deleted?

Yeah I'm not sure about this either, you're right that we don't have particularly well-defined guarantees in this area today. We do rely on prompt deletion/closure in some tests tho, enough that it seemed worth a conversation. Again, not intending to solve this here, will follow up.

DaveCTurner · 2024-05-01T21:20:51Z

I wonder if we can add an integration test to validate that closing shards no longer blocks the applier thread?

Possibly although between org.elasticsearch.index.shard.IndexShardTests#testAsyncCloseShard and org.elasticsearch.cluster.service.ClusterApplierService#assertNotApplyingClusterState I think this is fairly well covered already. Closing a shard can technically still block the applier thread in the exception-handling code at the bottom of org.elasticsearch.index.IndexService#createShard, although there can't be an engine to close at this point (already covered by assertions).

henningandersen

LGTM.

This method kinda only makes sense in the context of applying a specific cluster state, so with this commit we validate that the subscription matches the expected state. Also removes a couple of other TODOs that we've decided not to do. Relates elastic#108145

When a shard fails locally we remove it from the `IndicesService` and close it on a `GENERIC` thread, but do so while synchronized on the `IndicesClusterStateService`. With this commit we dispatch the work to close the shard into the background in order to release the mutex sooner. Relates elastic#108145

After elastic#108145, the after shard closed listener is no longer called on the cluster state applier thread. Introduce another event that is called on the applier thread. Relates elastic#108145

When a shard fails locally we remove it from the `IndicesService` and close it on a `GENERIC` thread, but do so while synchronized on the `IndicesClusterStateService`. With this commit we dispatch the work to close the shard into the background in order to release the mutex sooner. Relates #108145

After #108145, the after shard closed listener is no longer called on the cluster state applier thread. Introduce another event that is called on the applier thread. Relates #108145

The change in elastic#108145 means that the temporary engine created in `resetEngineToGlobalCheckpoint()` might now be closed on a thread that does not hold the `engineMutex`. This commit adds the missing synchronization.

The change in #108145 means that the temporary engine created in `resetEngineToGlobalCheckpoint()` might now be closed on a thread that does not hold the `engineMutex`. This commit adds the missing synchronization.

It seems that the hectic retrying with a new cluster state update just makes things worse. The initial deletion after the relocation might take a bit, I assume also more visible in this test because we've made shard close async in #108145. After that we just check once and if the shard dir is there we keep pushing new cluster states and checking again and this keeps failing with the check mentioned here. I've picked a simple solution since this is a test issue and just check a bit longer before triggering the new cluster state update. I've looked into a couple of other hooks (e.g. IndexFoldersDeletionListener#beforeIndexFoldersDeleted and org.elasticsearch.indices.cluster.IndicesClusterStateService#onClusterStateShardsClosed ) to see if we could rely on them rather than the assertBusy used here. None unfortunately seem to be cleanly allow getting rid of the assertBusy. IMO, since the shard store deletion is retried and guarantees to eventually work, to avoid flaky tests, we should still keep relying on the retries initiated by the cluster state update. I'll keep the issue open for a while before removing the extra logging. Running it locally has not failed. Relates #111134

It seems that the hectic retrying with a new cluster state update just makes things worse. The initial deletion after the relocation might take a bit, I assume also more visible in this test because we've made shard close async in elastic#108145. After that we just check once and if the shard dir is there we keep pushing new cluster states and checking again and this keeps failing with the check mentioned here. I've picked a simple solution since this is a test issue and just check a bit longer before triggering the new cluster state update. I've looked into a couple of other hooks (e.g. IndexFoldersDeletionListener#beforeIndexFoldersDeleted and org.elasticsearch.indices.cluster.IndicesClusterStateService#onClusterStateShardsClosed ) to see if we could rely on them rather than the assertBusy used here. None unfortunately seem to be cleanly allow getting rid of the assertBusy. IMO, since the shard store deletion is retried and guarantees to eventually work, to avoid flaky tests, we should still keep relying on the retries initiated by the cluster state update. I'll keep the issue open for a while before removing the extra logging. Running it locally has not failed. Relates elastic#111134

) It seems that the hectic retrying with a new cluster state update just makes things worse. The initial deletion after the relocation might take a bit, I assume also more visible in this test because we've made shard close async in #108145. After that we just check once and if the shard dir is there we keep pushing new cluster states and checking again and this keeps failing with the check mentioned here. I've picked a simple solution since this is a test issue and just check a bit longer before triggering the new cluster state update. I've looked into a couple of other hooks (e.g. IndexFoldersDeletionListener#beforeIndexFoldersDeleted and org.elasticsearch.indices.cluster.IndicesClusterStateService#onClusterStateShardsClosed ) to see if we could rely on them rather than the assertBusy used here. None unfortunately seem to be cleanly allow getting rid of the assertBusy. IMO, since the shard store deletion is retried and guarantees to eventually work, to avoid flaky tests, we should still keep relying on the retries initiated by the cluster state update. I'll keep the issue open for a while before removing the extra logging. Running it locally has not failed. Relates #111134 Co-authored-by: Elastic Machine <[email protected]>

DaveCTurner added >bug :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.15.0 labels May 1, 2024

elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 1, 2024

Update docs/changelog/108145.yaml

4c321e2

DaveCTurner mentioned this pull request May 1, 2024

Async close of IndexShard #107952

Closed

TODOs

13be4e9

DaveCTurner requested a review from henningandersen May 1, 2024 15:00

henningandersen reviewed May 1, 2024

View reviewed changes

DaveCTurner requested a review from henningandersen May 1, 2024 21:12

henningandersen approved these changes May 2, 2024

View reviewed changes

DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 3, 2024

Merge branch 'main' into 2024/05/01/IndexShard-async-close

eac6728

elasticsearchmachine merged commit f0ea058 into elastic:main May 3, 2024
15 checks passed

DaveCTurner deleted the 2024/05/01/IndexShard-async-close branch May 3, 2024 06:46

DaveCTurner mentioned this pull request May 3, 2024

Validate ICSS.onClusterStateShardsClosed calls #108240

Closed

DaveCTurner mentioned this pull request May 3, 2024

Avoid closing shard under ICSS mutex #108244

Merged

henningandersen mentioned this pull request May 6, 2024

Add shard closing listener #108317

Merged

idegtiarenko mentioned this pull request May 14, 2024

Fix testDanglingIndicesCanBeListed #108599

Merged

DaveCTurner mentioned this pull request May 15, 2024

Re-acquire engineMutex during engine reset #108680

Merged

DaveCTurner mentioned this pull request May 30, 2024

Exponential backoff of failed allocation #24530

Closed

DaveCTurner restored the 2024/05/01/IndexShard-async-close branch June 17, 2024 06:16

pxsalehi mentioned this pull request Sep 18, 2024

Unmute and fix PrevalidateShardPathIT#testCheckShards #113107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async close of `IndexShard` #108145

Async close of `IndexShard` #108145

DaveCTurner commented May 1, 2024

elasticsearchmachine commented May 1, 2024

elasticsearchmachine commented May 1, 2024

henningandersen left a comment

henningandersen May 1, 2024

DaveCTurner May 1, 2024

henningandersen May 1, 2024

DaveCTurner May 1, 2024

DaveCTurner commented May 1, 2024

henningandersen left a comment

Async close of IndexShard #108145

Async close of IndexShard #108145

Conversation

DaveCTurner commented May 1, 2024

elasticsearchmachine commented May 1, 2024

elasticsearchmachine commented May 1, 2024

henningandersen left a comment

Choose a reason for hiding this comment

henningandersen May 1, 2024

Choose a reason for hiding this comment

DaveCTurner May 1, 2024

Choose a reason for hiding this comment

henningandersen May 1, 2024

Choose a reason for hiding this comment

DaveCTurner May 1, 2024

Choose a reason for hiding this comment

DaveCTurner commented May 1, 2024

henningandersen left a comment

Choose a reason for hiding this comment

Async close of `IndexShard` #108145

Async close of `IndexShard` #108145