Do not access snapshot repo on dedicated voting-only master node #61016
Conversation
ywelsch
added
>enhancement
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
elasticmachine
added
the
Team:Distributed (Obsolete)
| public void applyClusterState(ClusterChangedEvent event) { | ||
| try { | ||
| final ClusterState state = event.state(); | ||
| if (isDedicatedVotingOnlyNode(state.nodes().getLocalNode())) { |
There was a problem hiding this comment.
Maybe we could just not add the applier in the first place in the constructor where we check if the node is a master or data node (we should have the information that this is a voting only node there already from the settings?)?
There was a problem hiding this comment.
We need to be able to compute this from a DiscoveryNode since we need to know it for remote nodes too, and we won't have a DiscoveryNode in the constructor so would have to implement it two different ways. I think I prefer this.
There was a problem hiding this comment.
yeah, I went back and forth on this, and prefer to reuse the same logic.
I have done this in a different way now in 143fc1c: Let me know if you prefer this.
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
One nit is that I'd like to spell out that it really is the right name for the role, but otherwise LGTM
| public void applyClusterState(ClusterChangedEvent event) { | ||
| try { | ||
| final ClusterState state = event.state(); | ||
| if (isDedicatedVotingOnlyNode(state.nodes().getLocalNode())) { |
There was a problem hiding this comment.
We need to be able to compute this from a DiscoveryNode since we need to know it for remote nodes too, and we won't have a DiscoveryNode in the constructor so would have to implement it two different ways. I think I prefer this.
|
|
||
| static boolean isDedicatedVotingOnlyNode(DiscoveryNode node) { | ||
| return node.isMasterNode() && node.isDataNode() == false && | ||
| node.getRoles().stream().anyMatch(role -> role.roleName().equals("voting_only")); |
There was a problem hiding this comment.
Can we establish that this is the same string as used in VotingOnlyNodePlugin#VOTING_ONLY_NODE_ROLE by extracting the string as a field and adding an assertion in VotingOnlyNodePlugin saying that they match?
There was a problem hiding this comment.
This is ensured by the test that was added. It fails when these strings are not matching (as we are then illegally accessing the repo on the voting-only node)? Do you find this too implicit?
) Today a snapshot repository verification ensures that all master-eligible and data nodes have write access to the snapshot repository (and can see each other's data) since taking a snapshot requires data nodes and the currently elected master to write to the repository. However, a dedicated voting-only master-eligible node is not a data node and will never be the elected master so we should not require it to have write access to the repository. Closes #59649
) Today a snapshot repository verification ensures that all master-eligible and data nodes have write access to the snapshot repository (and can see each other's data) since taking a snapshot requires data nodes and the currently elected master to write to the repository. However, a dedicated voting-only master-eligible node is not a data node and will never be the elected master so we should not require it to have write access to the repository. Closes #59649
5 participants
Today a snapshot repository verification ensures that all master-eligible and data nodes have write access to the snapshot repository (and can see each other's data) since taking a snapshot requires data nodes and the currently-elected master to write to the repository. However, a dedicated voting-only master-eligible node is not a data node and will never be the elected master so we should not require it to have write access to the repository.
Closes #59649