Skip peer discovery cleanup when backend returns error

Previously if the peer discovery backend returned an error from failing to discover nodes, the `service_discovery_nodes/0` helper returned an empty list. During cleanup this would mean that any nodes unreachable during a partition would have destructive action taken against them: `rabbit_db_cluster:forget_member/2` and `rabbit_quorum_queue:shrink_all/1`. The `list_nodes/0` callback can fail transiently, though, and a failure shouldn't mean that the cluster is empty. It's safer to avoid cleaning up any nodes when the peer discovery backend fails to return the intended set of nodes. (cherry picked from commit 2d4f19c)

Author: the-mikedavis

deps/rabbitmq_peer_discovery_common/src/rabbit_peer_discovery_cleanup.erl

@@ -240,19 +240,29 @@ maybe_cleanup(State, UnreachableNodes) ->
  ?LOG_DEBUG(
  "Peer discovery: cleanup discovered unreachable nodes: ~tp",
  [UnreachableNodes]),
- case lists:subtract(as_list(UnreachableNodes), as_list(service_discovery_nodes())) of
- [] ->
- ?LOG_DEBUG(
- "Peer discovery: all unreachable nodes are still "
- "registered with the discovery backend ~tp",
- [rabbit_peer_discovery:backend()],
- #{domain => ?RMQLOG_DOMAIN_PEER_DISC}),
- ok;
- Nodes ->
- ?LOG_DEBUG(
- "Peer discovery: unreachable nodes are not registered "
- "with the discovery backend ~tp", [Nodes]),
- maybe_remove_nodes(Nodes, State#state.warn_only)
+ Module = rabbit_peer_discovery:backend(),
+ case rabbit_peer_discovery:normalize(Module:list_nodes()) of
+ {ok, {OneOrMultipleNodes, _Type}} ->
+ DiscoveredNodes = as_list(OneOrMultipleNodes),
+ case lists:subtract(UnreachableNodes, DiscoveredNodes) of
+ [] ->
+ ?LOG_DEBUG(
+ "Peer discovery: all unreachable nodes are still "
+ "registered with the discovery backend ~tp",
+ [rabbit_peer_discovery:backend()],
+ #{domain => ?RMQLOG_DOMAIN_PEER_DISC}),
+ ok;
+ Nodes ->
+ ?LOG_DEBUG(
+ "Peer discovery: unreachable nodes are not registered "
+ "with the discovery backend ~tp", [Nodes]),
+ maybe_remove_nodes(Nodes, State#state.warn_only)
+ end;
+ {error, Reason} ->
+ ?LOG_INFO(
+ "Peer discovery cleanup: ~tp returned error ~tp",
+ [Module, Reason]),
+ ok
  end.
 
 %%--------------------------------------------------------------------
@@ -288,26 +298,3 @@ maybe_remove_nodes([Node | Nodes], false) ->
 -spec unreachable_nodes() -> [node()].
 unreachable_nodes() ->
  rabbit_nodes:list_unreachable().
-
-%%--------------------------------------------------------------------
-%% @private
-%% @doc Return the nodes that the service discovery backend knows about
-%% @spec service_discovery_nodes() -> [node()]
-%% @end
-%%--------------------------------------------------------------------
--spec service_discovery_nodes() -> [node()].
-service_discovery_nodes() ->
- Module = rabbit_peer_discovery:backend(),
- case rabbit_peer_discovery:normalize(Module:list_nodes()) of
- {ok, {OneOrMultipleNodes, _Type}} ->
- Nodes = as_list(OneOrMultipleNodes),
- ?LOG_DEBUG(
- "Peer discovery cleanup: ~tp returned ~tp",
- [Module, Nodes]),
- Nodes;
- {error, Reason} ->
- ?LOG_DEBUG(
- "Peer discovery cleanup: ~tp returned error ~tp",
- [Module, Reason]),
- []
- end.

deps/rabbitmq_peer_discovery_common/test/peer_discovery_cleanup_SUITE.erl

@@ -26,7 +26,8 @@ groups() ->
 
 all_tests() ->
  [
- cleanup_queues
+ cleanup_queues,
+ backend_errors_do_not_trigger_cleanup
  ].
 
 %% -------------------------------------------------------------------
@@ -126,6 +127,45 @@ cleanup_queues(Config) ->
  %% Cleanup.
  ok = rabbit_ct_client_helpers:close_connection_and_channel(Conn, Ch).
 
+backend_errors_do_not_trigger_cleanup(Config) ->
+ %% The backend could have some transient failures. While the backend is
+ %% not giving reliable peer information, skip cleanup.
+ [A, B, C] = rabbit_ct_broker_helpers:get_node_configs(Config, nodename),
+ {Conn, Ch} = rabbit_ct_client_helpers:open_connection_and_channel(Config),
+
+ QQ = <<"quorum-queue">>,
+ declare_queue(Ch, QQ, [{<<"x-queue-type">>, longstr, <<"quorum">>}]),
+
+ %% Have the backend return an error.
+ mock_list_nodes(Config, {error, "...internal server error..."}),
+ %% Make node C unreachable.
+ rabbit_ct_broker_helpers:block_traffic_between(A, C),
+ rabbit_ct_broker_helpers:block_traffic_between(B, C),
+ Ts1 = erlang:system_time(millisecond),
+ ?awaitMatch([C],
+ rabbit_ct_broker_helpers:rpc(Config, A,
+ rabbit_nodes,
+ list_unreachable, []),
+ 30_000, 1_000),
+ ct:log(?LOW_IMPORTANCE, "Node C became unreachable in ~bms",
+ [erlang:system_time(millisecond) - Ts1]),
+
+ ok = rabbit_ct_broker_helpers:rpc(Config, A,
+ rabbit_peer_discovery_cleanup,
+ check_cluster, []),
+
+ %% Node C should remain in the quorum queue members.
+ ?assertEqual(
+ lists:sort([A, B, C]),
+ begin
+ Info = rpc:call(A, rabbit_quorum_queue, infos,
+ [rabbit_misc:r(<<"/">>, queue, QQ)]),
+ lists:sort(proplists:get_value(members, Info))
+ end),
+
+ %% Cleanup.
+ ok = rabbit_ct_client_helpers:close_connection_and_channel(Conn, Ch).
+
 %%%
 %%% Implementation
 %%%