IndicesStore shouldn't try to delete index after deleting a shard

[imotov]

When a node discovers shard content on disk which isn't used, we reach out to all other nodes that supposed to have the shard active. Only once all of those have confirmed the shard active, the shard has no unassigned copies and no cluster state change have happened in the mean while, do we go and delete the shard folder.

Currently, after removing a shard, the IndicesStores checks the indices services if that has no more shard active for this index and if so, it tries to delete the entire index folder (unless on master node, where we keep the index metadata around). This is wrong as both the check and the protections in IndicesServices.deleteIndexStore make sure that there isn't any shard in use from that index. However, it may be the we erroneously delete other unused shard copies on disk, without the proper safety guards described above.

Normally, this is not a problem as the missing copy will be recovered from another shard copy on another node (although a shame). However, in extremely rare cases involving multiple node failures/restarts where all shard copies are not available (i.e., shard is red) there are race conditions which can cause all shard copies to be deleted.

Instead, we should change the decision to clean up an index folder to be based on checking the index directory for being empty and containing no shards.

Note: this PR is against the 1.6 branch.

[dakrone]

LGTM

[s1monw]

to me it seems like that some of the safety that has been added here is still prone to concurrency issues. We for instance check this.indices in canDeleteIndexContents which can be concurrently modified by a createIndex operation so I think there are still races in this code that could cause problems?

[imotov]

@s1monw to me it looked like all these calls are performed on updateTask thread so they shouldn't run into concurrency issues. What did I miss?

[s1monw]

for instance

public boolean canDeleteIndexContents(Index index, Settings indexSettings) {
        final Tuple<IndexService, Injector> indexServiceInjectorTuple = this.indices.get(index.name());
        if (IndexMetaData.isOnSharedFilesystem(indexSettings) == false) {
            if (indexServiceInjectorTuple == null && nodeEnv.hasNodeFile()) {
                return true;
            }
        } else {
            logger.trace("{} skipping index directory deletion due to shadow replicas", index);
        }
        return false;
    }

checks if the index is present but it could be created concurrently such that it can potentially check before it's created but delete after it's creation was successful?

[imotov]

@s1monw concurrently on which thread?

[s1monw]

well we can call thsi API from everywhere I wonder if we should synchronize all these operations? there are pending deletes threads etc that are kicked off so I really thing we should protect that.

[bleskes]

+1 to not relaying on the methods to be called from the cluster state update thread. If I understand the concern correctly, It relates to an index being deleted and created concurrently. In that case I think the shard locking in NodeEnvironment will protect us from deleting data. However, I agree it would be nice to have clear concurrency semantics on this level of the code. Right now we sometimes synchronize and sometimes not which make it hard to reason about- that is potentially a much bigger change and I was trying to make the smallest intervention. @s1monw if I misunderstood what you said and there is a concrete hole in the logic - can you open an issue so we won't forget?