Drop node if asymmetrically partitioned from master #39598
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a node is joining the cluster we ensure that it can send requests to the master _at that time_. If it joins the cluster and _then_ loses the ability to send requests to the master then it should be removed from the cluster. Today this is not the case: the master can still receive responses to its follower checks, and receives acknowledgements to cluster state publications, so has no reason to remove the node. This commit changes the handling of follower checks so that they fail if they come from a master that the other node was following but which it now believes to have failed.
DaveCTurner
added
>bug
v7.0.0
:Distributed Coordination/Cluster Coordination
|
Pinging @elastic/es-distributed |
|
@elasticmachine run elasticsearch-ci/bwc |
…ub.com:DaveCTurner/elasticsearch into 2019-03-02-remove-node-on-asymmetric-partition
ywelsch
approved these changes
Mar 5, 2019
| }); | ||
| boolean isJoinPending() { | ||
| // cannot use pendingOutgoingJoins.isEmpty() because it's not properly synchronized. | ||
| return pendingOutgoingJoins.iterator().hasNext(); |
There was a problem hiding this comment.
I don't understand how using the iterator gives you any stronger guarantees.
There was a problem hiding this comment.
The ConcurrentHashMap javadocs say:
* Bear in mind that the results of aggregate status methods including
* {@code size}, {@code isEmpty}, and {@code containsValue} are typically
* useful only when a map is not undergoing concurrent updates in other threads.
* Otherwise the results of these methods reflect transient states
* that may be adequate for monitoring or estimation purposes, but not
* for program control.
There was a problem hiding this comment.
I believe the weakly consistent only guarantee given by ConcurrentHashMap iteration means that following could happen:
pendingOutgoingJoinshas one entry,e.- A thread
T1callsisJoinPendingand gets the iterator (strictly speaking, we halt inside iterator construction beforeadvanceis called). - A thread
T2adds another entry f and removes e (in that order). SopendingOutgoingJoinswas never empty. T1continues andhasNext()can now returnfalse.
Mutex protection is done in Coordinator, but is not applied upon receiving requests and responses. It is difficult to see if above scenario can lead to issues, but a simpler solution could be to make the pendingOutgoingJoins set synchronized instead (or maybe synchronize on the Coordinator.mutex when manipulating it)?
There was a problem hiding this comment.
Also from the Javadocs:
* Iterators, Spliterators and Enumerations return elements reflecting the
* state of the hash table at some point at or since the creation of the
* iterator/enumeration.
The at some point in that sentence indicates a snapshot-like semantics which would forbid this situation, and also the consequences are benign as far as I can see, but I'm all for reducing unnecessary mental load. I opened #39900.
| @@ -1560,6 +1596,14 @@ void setEmptySeedHostsList() { | |||
| seedHostsList = emptyList(); | |||
| } | |||
|
|
|||
| void dropRequestsFrom(ClusterNode sender, ClusterNode destination) { | |||
There was a problem hiding this comment.
perhaps call this blackHoleRequestsFrom?
DaveCTurner
added a commit
that referenced
this pull request
Mar 6, 2019
When a node is joining the cluster we ensure that it can send requests to the master _at that time_. If it joins the cluster and _then_ loses the ability to send requests to the master then it should be removed from the cluster. Today this is not the case: the master can still receive responses to its follower checks, and receives acknowledgements to cluster state publications, so has no reason to remove the node. This commit changes the handling of follower checks so that they fail if they come from a master that the other node was following but which it now believes to have failed.
DaveCTurner
added a commit
that referenced
this pull request
Mar 6, 2019
When a node is joining the cluster we ensure that it can send requests to the master _at that time_. If it joins the cluster and _then_ loses the ability to send requests to the master then it should be removed from the cluster. Today this is not the case: the master can still receive responses to its follower checks, and receives acknowledgements to cluster state publications, so has no reason to remove the node. This commit changes the handling of follower checks so that they fail if they come from a master that the other node was following but which it now believes to have failed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a node is joining the cluster we ensure that it can send requests to the
master at that time. If it joins the cluster and then loses the ability to
send requests to the master then it should be removed from the cluster. Today
this is not the case: the master can still receive responses to its follower
checks, and receives acknowledgements to cluster state publications, so has no
reason to remove the node.
This commit changes the handling of follower checks so that they fail if they
come from a master that the other node was following but which it now believes
to have failed.