Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Index deletes not applied when cluster UUID has changed #16825

Closed
wants to merge 1 commit into from
Closed

Index deletes not applied when cluster UUID has changed #16825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
100 changes: 65 additions & 35 deletions core/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java
View file
Open in desktop
Expand Up @@ -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 {

Expand All @@ -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;
Expand All @@ -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;
}
Expand All @@ -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();
}
Expand All @@ -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;
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a note saying this is an reference level equality and not a true equal.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I also added that note to every other reference equality check in the class

* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here re type of equality.

* 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) {
Expand All @@ -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;
}
}
}
13 changes: 9 additions & 4 deletions core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java
View file
Open in desktop
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}
Expand All @@ -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);
}

Expand All @@ -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();
}
Expand All @@ -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);
}

Expand Down