Shadow Replica indexes do not delete properly

[abeyad]

This issue is to document deletion problems with shadow replica indices that were found while working on #17265. A separate PR #17638 that improves the naming of methods in the IndicesService also contains tests or added assertions to existing tests that reveal the issues below and must be enabled as part of any PR that fixes the issues.

No. 1
The index file deletion logic that is triggered in IndicesService#deleteIndexStore(String reason, Index index, IndexSettings indexSettings checks before deleting files to see if the index is not a shadow replica, or if it is, ensure that it has been closed before (so that no other nodes are holding resources to it). An issue with this is that it is too strict of a check, so that if a shadow replica index is deleted, if it was not previously closed, the index folder itself is not deleted and remains on the file system (an empty folder). So one of the issues that needs fixing is to ensure index directories are deleted even on shadow replica index deletes. The following tests have commented out assertions to test this behavior once fixed:

• IndexWithShadowReplicaIT#testIndexWithShadowReplicasCleansUp
• IndexWithShadowReplicaIT#testShadowReplicaNaturalRelocation

Note that shared shard data is cleaned up properly in a shadow replica index that is not closed, as the shard data is deleted by the StoreCloseListener. This is verified in the tests with the assertPathHasBeenCleared assert.

No. 2
The issue with deleting a shadow replica index that was previously closed is that all of the index and shard data are potentially deleted simultaneously by each node that receives the delete operation and invokes NodeEnvironment#deleteIndexDirectorySafe. This can lead to race conditions where a node is trying to delete a file that was deleted by another node as both are walking the file system simultaneously (using Lucene's IOUtils.rm). This ends up logged as a warning in IndicesService#deleteIndexStore(String reason, Index index, IndexSettings indexSettings and the deletion is put on the pending queue.

[bleskes]

none of its index files are deleted

To be clear - shard level folders are deleted and the index metadata is deleted as well. The problem is that we leave an empty folder behind.

Re No 2.:

Do we have specific issue with master nodes or is it specifically about nodes that do not host a shard of the shard from the shadow index? (i.e., master nodes but also some data nodes)

[abeyad]

To be clear - shard level folders are deleted and the index metadata is deleted as well. The problem is that we leave an empty folder behind.

Yes, that is correct. I changed the description to be more clear on this.

Do we have specific issue with master nodes or is it specifically about nodes that do not host a shard of the shard from the shadow index? (i.e., master nodes but also some data nodes)

I think our tests prove that this isn't a specific issue, but only a general one as it relates to index folders not getting cleaned up. Since we are dropping the testDeletingIndexWithDedicatedMasterNodes, I will remove this point from the description as well.

I believe the fundamental problems with shadow replica deletion are 1. the leaving behind of these (empty) index folders and 2. when a shadow replica has been closed and then deleted, all nodes try to delete the same shared shard data folder, in which case, the first node succeeds, but the remaining nodes try to delete already deleted files (with the potential for race conditions here if multiple nodes are deleting at the same time). This throws a warning in the logs and puts the delete in the pending queue.

I will update the description of this issue accordingly.

[bleskes]

@abeyad was there any discussion on this ? if not we should pick it and make a plan.

[abeyad]

@bleskes nothing further has been done on this, its worth a discussion to see how best to handle the aforementioned scenarios.

[abeyad]

Upon discussion with Boaz, these issues are known and have been there for a long time. They don't cause incorrect behavior, they just prevent shadow replicas from deleting cleanly. It remains to be discussed if this is important to tackle and if so, what the appropriate solutions are.

@bleskes @clintongormley FYI

[dakrone]

@abeyad @clintongormley how do we feel about this being a blocker for 5.0? It's currently marked as a blocker due to a // norelease in IndexWithShadowReplicasIT pointing to this issue

[abeyad]

I'm inclined to say these are not blockers, because they result in improper full cleanup (empty directories laying around) or simultaneous deletes that are logged as such without tripping any errors. Also, adding the failed deletes to the pending queue just means they will get executed later and essentially be no-ops because there will be nothing to delete (as the deletion was already done successfully by one of the nodes).

So, its not great form, but given that shadow replicas aren't a ubiquitous feature and this issue doesn't actually cause incorrect behavior, I don't feel its a blocker.

If @clintongormley agrees, I can remove the the // norelease from the test.

@dakrone what do you think?

[clintongormley]

+1

[abeyad]

I removed the blocker label and removed the //norelease in 50b31ce

[javanna]

I would close this now that shadow replicas have been removed. I do not think we will go back to old branches and fix this problem.