Don't die in case of faulty node #894
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
This patch fixes TOCTOU issue introduced in the following commit: * rabbitmq/rabbitmq-server@93b9e37 If the node was just removed from the cluster, then there is a small window when it is still listed as a member of a Mnesia cluster locally. We retrieve list of nodes by calling locally ```erlang unsafe_rpc(Node, rabbit_mnesia, cluster_nodes, [running]). ``` However retrieving status from that particular failed node is no longer available and throws an exception. See `alarms_by_node(Name)` function, which is simply calls `unsafe_rpc(Name, rabbit, status, [])` for this node. This `unsafe_rpc/4` function is basically a wrapper over `rabbit_misc:rpc_call/4` which translates `{badrpc, nodedown}` into exception. This exception generated by `alarms_by_node(Name)` function call emerges on a very high level, so rabbitmqct thinks that the entire cluster is down, while generating a very bizarre message: Cluster status of node 'rabbit@overcloud-controller-0' ... Error: unable to connect to node 'rabbit@overcloud-controller-0': nodedown DIAGNOSTICS =========== attempted to contact: ['rabbit@overcloud-controller-0'] rabbit@overcloud-controller-0: * connected to epmd (port 4369) on overcloud-controller-0 * node rabbit@overcloud-controller-0 up, 'rabbit' application running current node details: - node name: 'rabbitmq-cli-31@overcloud-controller-0' - home dir: /var/lib/rabbitmq - cookie hash: PB31uPq3vzeQeZ+MHv+wgg== See - it reports that it failed to connect to node 'rabbit@overcloud-controller-0' (because it catches an exception from `alarms_by_node(Name)`), but attempt to connect to this node was successful ('rabbit' application running). In order to fix that we should not throw exception during consequent calls (`[alarms_by_node(Name) || Name <- nodes_in_cluster(Node)]`), only during the first one (`unsafe_rpc(Node, rabbit_mnesia, status, [])`). Even more - we don't need to change `nodes_in_cluster(Node)`, because it is called locally. The only function which must use `rabbit_misc:rpc_call/4` is `alarms_by_node(Name)` because it is executed remotely. See this issue for further details and real world example: * https://bugzilla.redhat.com/1356169 Signed-off-by: Peter Lemenkov <[email protected]>
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.
This patch fixes TOCTOU issue introduced in the following commit:
If the node was just removed from the cluster, then there is a small
window when it is still listed as a member of a Mnesia cluster
locally. We retrieve list of nodes by calling locally
However retrieving status from that particular failed node is no longer
available and throws an exception. See
alarms_by_node(Name)function,which is simply calls
unsafe_rpc(Name, rabbit, status, [])for thisnode.
This
unsafe_rpc/4function is basically a wrapper overrabbit_misc:rpc_call/4which translates
{badrpc, nodedown}into exception. This exceptiongenerated by
alarms_by_node(Name)function call emerges on a very highlevel, so rabbitmqct thinks that the entire cluster is down, while
generating a very bizarre message:
See - it reports that it failed to connect to node
'rabbit@overcloud-controller-0' (because it catches an exception from
alarms_by_node(Name)), but attempt to connect to this node wassuccessful ('rabbit' application running).
In order to fix that we should not throw exception during consequent
calls (
[alarms_by_node(Name) || Name <- nodes_in_cluster(Node)]), onlyduring the first one (
unsafe_rpc(Node, rabbit_mnesia, status, [])).Even more - we don't need to change
nodes_in_cluster(Node), because itis called locally. The only function which must use
rabbit_misc:rpc_call/4isalarms_by_node(Name)because it isexecuted remotely.
See this issue for further details and real world example:
Signed-off-by: Peter Lemenkov [email protected]