Index deletes not applied when cluster UUID has changed

If a node was isolated from the cluster while a delete was happening,
the node will ignore the deleted operation when rejoining as we couldn't
detect whether the new master genuinely deleted the indices or it is a
new fresh "reset" master that was started without the old data folder.
We can now be smarter and detect these reset masters and actually delete
the indices on the node if its not the case of a reset master.

Note that this new protection doesn't hold if the node was shut down. In
that case it's indices will still be imported as dangling indices.

Closes #16825
Closes #11665

Author: Ali Beyad

core/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java

@@ -25,12 +25,12 @@
 import org.elasticsearch.cluster.node.DiscoveryNodes;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 
 /**
- *
+ * An event received by the local node, signaling that the cluster state has changed.
  */
 public class ClusterChangedEvent {
 
@@ -43,6 +43,9 @@ public class ClusterChangedEvent {
     private final DiscoveryNodes.Delta nodesDelta;
 
     public ClusterChangedEvent(String source, ClusterState state, ClusterState previousState) {
+        Objects.requireNonNull(source, "source must not be null");
+        Objects.requireNonNull(state, "state must not be null");
+        Objects.requireNonNull(previousState, "previousState must not be null");
         this.source = source;
         this.state = state;
         this.previousState = previousState;
@@ -56,19 +59,35 @@ public String source() {
         return this.source;
     }
 
+    /**
+     * The new cluster state that caused this change event.
+     */
     public ClusterState state() {
         return this.state;
     }
 
+    /**
+     * The previous cluster state for this change event.
+     */
     public ClusterState previousState() {
         return this.previousState;
     }
 
+    /**
+     * Returns <code>true</code> iff the routing tables (for all indices) have
+     * changed between the previous cluster state and the current cluster state.
+     * Note that this is an object reference equality test, not an equals test.
+     */
     public boolean routingTableChanged() {
         return state.routingTable() != previousState.routingTable();
     }
 
+    /**
+     * Returns <code>true</code> iff the routing table has changed for the given index.
+     * Note that this is an object reference equality test, not an equals test.
+     */
     public boolean indexRoutingTableChanged(String index) {
+        Objects.requireNonNull(index, "index must not be null");
         if (!state.routingTable().hasIndex(index) && !previousState.routingTable().hasIndex(index)) {
             return false;
         }
@@ -82,9 +101,6 @@ public boolean indexRoutingTableChanged(String index) {
      * Returns the indices created in this event
      */
     public List<String> indicesCreated() {
-        if (previousState == null) {
-            return Arrays.asList(state.metaData().indices().keys().toArray(String.class));
-        }
         if (!metaDataChanged()) {
             return Collections.emptyList();
         }
@@ -105,20 +121,14 @@ public List<String> indicesCreated() {
      * Returns the indices deleted in this event
      */
     public List<String> indicesDeleted() {
-
-        // if the new cluster state has a new master then we cannot know if an index which is not in the cluster state
-        // is actually supposed to be deleted or imported as dangling instead. for example a new master might not have
-        // the index in its cluster state because it was started with an empty data folder and in this case we want to
-        // import as dangling. we check here for new master too to be on the safe side in this case.
-        // This means that under certain conditions deleted indices might be reimported if a master fails while the deletion
-        // request is issued and a node receives the cluster state that would trigger the deletion from the new master.
-        // See test MetaDataWriteDataNodesTests.testIndicesDeleted()
+        // If the new cluster state has a new cluster UUID, the likely scenario is that a node was elected
+        // master that has had its data directory wiped out, in which case we don't want to delete the indices and lose data;
+        // rather we want to import them as dangling indices instead.  So we check here if the cluster UUID differs from the previous
+        // cluster UUID, in which case, we don't want to delete indices that the master erroneously believes shouldn't exist.
+        // See test DiscoveryWithServiceDisruptionsIT.testIndicesDeleted()
         // See discussion on https://github.com/elastic/elasticsearch/pull/9952 and
         // https://github.com/elastic/elasticsearch/issues/11665
-        if (hasNewMaster() || previousState == null) {
-            return Collections.emptyList();
-        }
-        if (!metaDataChanged()) {
+        if (metaDataChanged() == false || isNewCluster()) {
             return Collections.emptyList();
         }
         List<String> deleted = null;
@@ -134,10 +144,20 @@ public List<String> indicesDeleted() {
         return deleted == null ? Collections.<String>emptyList() : deleted;
     }
 
+    /**
+     * Returns <code>true</code> iff the metadata for the cluster has changed between
+     * the previous cluster state and the new cluster state. Note that this is an object
+     * reference equality test, not an equals test.
+     */
     public boolean metaDataChanged() {
         return state.metaData() != previousState.metaData();
     }
 
+    /**
+     * Returns <code>true</code> iff the {@link IndexMetaData} for a given index
+     * has changed between the previous cluster state and the new cluster state.
+     * Note that this is an object reference equality test, not an equals test.
+     */
     public boolean indexMetaDataChanged(IndexMetaData current) {
         MetaData previousMetaData = previousState.metaData();
         if (previousMetaData == null) {
@@ -152,46 +172,56 @@ public boolean indexMetaDataChanged(IndexMetaData current) {
         return true;
     }
 
+    /**
+     * Returns <code>true</code> iff the cluster level blocks have changed between cluster states.
+     * Note that this is an object reference equality test, not an equals test.
+     */
     public boolean blocksChanged() {
         return state.blocks() != previousState.blocks();
     }
 
+    /**
+     * Returns <code>true</code> iff the local node is the mater node of the cluster.
+     */
     public boolean localNodeMaster() {
         return state.nodes().localNodeMaster();
     }
 
+    /**
+     * Returns the {@link org.elasticsearch.cluster.node.DiscoveryNodes.Delta} between
+     * the previous cluster state and the new cluster state.
+     */
     public DiscoveryNodes.Delta nodesDelta() {
         return this.nodesDelta;
     }
 
+    /**
+     * Returns <code>true</code> iff nodes have been removed from the cluster since the last cluster state.
+     */
     public boolean nodesRemoved() {
         return nodesDelta.removed();
     }
 
+    /**
+     * Returns <code>true</code> iff nodes have been added from the cluster since the last cluster state.
+     */
     public boolean nodesAdded() {
         return nodesDelta.added();
     }
 
+    /**
+     * Returns <code>true</code> iff nodes have been changed (added or removed) from the cluster since the last cluster state.
+     */
     public boolean nodesChanged() {
         return nodesRemoved() || nodesAdded();
     }
 
-    /**
-     * Checks if this cluster state comes from a different master than the previous one.
-     * This is a workaround for the scenario where a node misses a cluster state  that has either
-     * no master block or state not recovered flag set. In this case we must make sure that
-     * if an index is missing from the cluster state is not deleted immediately but instead imported
-     * as dangling. See discussion on https://github.com/elastic/elasticsearch/pull/9952
-     */
-    private boolean hasNewMaster() {
-        String oldMaster = previousState().getNodes().masterNodeId();
-        String newMaster = state().getNodes().masterNodeId();
-        if (oldMaster == null && newMaster == null) {
-            return false;
-        }
-        if (oldMaster == null && newMaster != null) {
-            return true;
-        }
-        return oldMaster.equals(newMaster) == false;
+    // Determines whether or not the current cluster state represents an entirely
+    // different cluster from the previous cluster state, which will happen when a
+    // master node is elected that has never been part of the cluster before.
+    private boolean isNewCluster() {
+        final String prevClusterUUID = previousState.metaData().clusterUUID();
+        final String currClusterUUID = state.metaData().clusterUUID();
+        return prevClusterUUID.equals(currClusterUUID) == false;
     }
-}
+}

core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java

@@ -46,6 +46,11 @@
  */
 public class DiscoveryNode implements Streamable, ToXContent {
 
+    public static final String DATA_ATTR = "data";
+    public static final String MASTER_ATTR = "master";
+    public static final String CLIENT_ATTR = "client";
+    public static final String INGEST_ATTR = "ingest";
+
     public static boolean localNode(Settings settings) {
         if (Node.NODE_LOCAL_SETTING.exists(settings)) {
             return Node.NODE_LOCAL_SETTING.get(settings);
@@ -274,7 +279,7 @@ public ImmutableOpenMap<String, String> getAttributes() {
      * Should this node hold data (shards) or not.
      */
     public boolean dataNode() {
-        String data = attributes.get("data");
+        String data = attributes.get(DATA_ATTR);
         if (data == null) {
             return !clientNode();
         }
@@ -292,7 +297,7 @@ public boolean isDataNode() {
      * Is the node a client node or not.
      */
     public boolean clientNode() {
-        String client = attributes.get("client");
+        String client = attributes.get(CLIENT_ATTR);
         return client != null && Booleans.parseBooleanExact(client);
     }
 
@@ -304,7 +309,7 @@ public boolean isClientNode() {
      * Can this node become master or not.
      */
     public boolean masterNode() {
-        String master = attributes.get("master");
+        String master = attributes.get(MASTER_ATTR);
         if (master == null) {
             return !clientNode();
         }
@@ -322,7 +327,7 @@ public boolean isMasterNode() {
      * Returns a boolean that tells whether this an ingest node or not
      */
     public boolean isIngestNode() {
-        String ingest = attributes.get("ingest");
+        String ingest = attributes.get(INGEST_ATTR);
         return ingest == null ? true : Booleans.parseBooleanExact(ingest);
     }