From c0be3eef3f79824a6d217d4eaca21f870bfd7eb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-S=C3=A9bastien=20P=C3=A9dron?= Date: Wed, 11 Oct 2023 17:58:31 +0200 Subject: [PATCH] rabbit_feature_flags: New `check_node_compatibility/2` variant ... that considers the local node as if it was reset. [Why] When a node joins a cluster, we check its compatibility with the cluster, reset the node, copy the feature flags states from the remote cluster and add that node to the cluster. However, the compatibility check is performed with the current feature flags states, even though they are about to be reset. Therefore, a node with an enabled feature flag that is unsupported by the cluster will refuse to join. It's incorrect because after the reset and the states copy, it could have join the cluster just fine. [How] We introduce a new variant of `check_node_compatibility/2` that takes an argument to indicate if the local node should be considered as a virgin node (i.e. like after a reset). This way, the joining node will always be able to join, regardless of its initial feature flags states, as long as it doesn't require a feature flag that is unsupported by the cluster. This also removes the need to use `$RABBITMQ_FEATURE_FLAGS` environment variable to force a new node to leave stable feature flags disabled to allow it to join a cluster running an older version. References #9677. --- deps/rabbit/src/rabbit_db_cluster.erl | 2 +- deps/rabbit/src/rabbit_feature_flags.erl | 41 ++++++++++++-- deps/rabbit/src/rabbit_ff_controller.erl | 69 +++++++++++++++++++----- 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/deps/rabbit/src/rabbit_db_cluster.erl b/deps/rabbit/src/rabbit_db_cluster.erl index 6988908ee8d9..12becd3923a2 100644 --- a/deps/rabbit/src/rabbit_db_cluster.erl +++ b/deps/rabbit/src/rabbit_db_cluster.erl @@ -62,7 +62,7 @@ can_join(RemoteNode) -> "DB: checking if `~ts` can join cluster using remote node `~ts`", [node(), RemoteNode], #{domain => ?RMQLOG_DOMAIN_DB}), - case rabbit_feature_flags:check_node_compatibility(RemoteNode) of + case rabbit_feature_flags:check_node_compatibility(RemoteNode, true) of ok -> case rabbit_khepri:is_enabled(RemoteNode) of true -> can_join_using_khepri(RemoteNode); diff --git a/deps/rabbit/src/rabbit_feature_flags.erl b/deps/rabbit/src/rabbit_feature_flags.erl index fc9578a932ed..3a1c2397ff3d 100644 --- a/deps/rabbit/src/rabbit_feature_flags.erl +++ b/deps/rabbit/src/rabbit_feature_flags.erl @@ -103,7 +103,7 @@ init/0, get_state/1, get_stability/1, - check_node_compatibility/1, + check_node_compatibility/1, check_node_compatibility/2, sync_feature_flags_with_cluster/2, refresh_feature_flags_after_app_load/0, enabled_feature_flags_list_file/0 @@ -1292,7 +1292,9 @@ does_node_support(Node, FeatureNames, Timeout) -> false end. --spec check_node_compatibility(node()) -> ok | {error, any()}. +-spec check_node_compatibility(RemoteNode) -> Ret when + RemoteNode :: node(), + Ret :: ok | {error, any()}. %% @doc %% Checks if a node is compatible with the local node. %% @@ -1304,11 +1306,40 @@ does_node_support(Node, FeatureNames, Timeout) -> %% local node %% %% -%% @param Node the name of the remote node to test. +%% @param RemoteNode the name of the remote node to test. +%% @returns `ok' if they are compatible, `{error, Reason}' if they are not. + +check_node_compatibility(RemoteNode) -> + check_node_compatibility(RemoteNode, false). + +-spec check_node_compatibility(RemoteNode, LocalNodeAsVirgin) -> Ret when + RemoteNode :: node(), + LocalNodeAsVirgin :: boolean(), + Ret :: ok | {error, any()}. +%% @doc +%% Checks if a node is compatible with the local node. +%% +%% To be compatible, the following two conditions must be met: +%%
    +%%
  1. feature flags enabled on the local node must be supported by the +%% remote node
    2. +%%
  2. feature flags enabled on the remote node must be supported by the +%% local node
    3. +%%
+%% +%% Unlike {@link check_node_compatibility/1}, the local node's feature flags +%% inventory is evaluated as if the node was virgin if `LocalNodeAsVirgin' is +%% true. This is useful if the local node will be reset as part of joining a +%% remote cluster for instance. +%% +%% @param RemoteNode the name of the remote node to test. +%% @param LocalNodeAsVirgin flag to indicate if the local node should be +%% evaluated as if it was virgin. %% @returns `ok' if they are compatible, `{error, Reason}' if they are not. -check_node_compatibility(Node) -> - rabbit_ff_controller:check_node_compatibility(Node). +check_node_compatibility(RemoteNode, LocalNodeAsVirgin) -> + rabbit_ff_controller:check_node_compatibility( + RemoteNode, LocalNodeAsVirgin). run_feature_flags_mod_on_remote_node(Node, Function, Args, Timeout) -> rabbit_ff_controller:rpc_call(Node, ?MODULE, Function, Args, Timeout). diff --git a/deps/rabbit/src/rabbit_ff_controller.erl b/deps/rabbit/src/rabbit_ff_controller.erl index ee20da822129..af88c125bdae 100644 --- a/deps/rabbit/src/rabbit_ff_controller.erl +++ b/deps/rabbit/src/rabbit_ff_controller.erl @@ -36,7 +36,7 @@ -export([is_supported/1, is_supported/2, enable/1, enable_default/0, - check_node_compatibility/1, + check_node_compatibility/2, sync_cluster/1, refresh_after_app_load/0, get_forced_feature_flag_names/0]). @@ -127,12 +127,22 @@ enable_default() -> Ret end. -check_node_compatibility(RemoteNode) -> +check_node_compatibility(RemoteNode, LocalNodeAsVirgin) -> ThisNode = node(), - ?LOG_DEBUG( - "Feature flags: CHECKING COMPATIBILITY between nodes `~ts` and `~ts`", - [ThisNode, RemoteNode], - #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), + case LocalNodeAsVirgin of + true -> + ?LOG_DEBUG( + "Feature flags: CHECKING COMPATIBILITY between nodes `~ts` " + "and `~ts`; consider node `~ts` as virgin", + [ThisNode, RemoteNode, ThisNode], + #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}); + false -> + ?LOG_DEBUG( + "Feature flags: CHECKING COMPATIBILITY between nodes `~ts` " + "and `~ts`", + [ThisNode, RemoteNode], + #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}) + end, %% We don't go through the controller process to check nodes compatibility %% because this function is used while `rabbit' is stopped usually. %% @@ -140,7 +150,7 @@ check_node_compatibility(RemoteNode) -> %% because it would not guaranty that the compatibility remains true after %% this function finishes and before the node starts and synchronizes %% feature flags. - check_node_compatibility_task(ThisNode, RemoteNode). + check_node_compatibility_task(ThisNode, RemoteNode, LocalNodeAsVirgin). sync_cluster(Nodes) -> ?LOG_DEBUG( @@ -363,12 +373,14 @@ notify_waiting_controller({ControlerPid, _} = From) -> %% Code to check compatibility between nodes. %% -------------------------------------------------------------------- --spec check_node_compatibility_task(Node, Node) -> Ret when - Node :: node(), +-spec check_node_compatibility_task(NodeA, NodeB, NodeAAsVirigin) -> Ret when + NodeA :: node(), + NodeB :: node(), + NodeAAsVirigin :: boolean(), Ret :: ok | {error, Reason}, Reason :: incompatible_feature_flags. -check_node_compatibility_task(NodeA, NodeB) -> +check_node_compatibility_task(NodeA, NodeB, NodeAAsVirigin) -> ?LOG_NOTICE( "Feature flags: checking nodes `~ts` and `~ts` compatibility...", [NodeA, NodeB], @@ -381,7 +393,8 @@ check_node_compatibility_task(NodeA, NodeB) -> _ when is_list(NodesB) -> check_node_compatibility_task1( NodeA, NodesA, - NodeB, NodesB); + NodeB, NodesB, + NodeAAsVirigin); Error -> ?LOG_WARNING( "Feature flags: " @@ -400,10 +413,12 @@ check_node_compatibility_task(NodeA, NodeB) -> {error, {aborted_feature_flags_compat_check, Error}} end. -check_node_compatibility_task1(NodeA, NodesA, NodeB, NodesB) +check_node_compatibility_task1(NodeA, NodesA, NodeB, NodesB, NodeAAsVirigin) when is_list(NodesA) andalso is_list(NodesB) -> case collect_inventory_on_nodes(NodesA) of - {ok, InventoryA} -> + {ok, InventoryA0} -> + InventoryA = virtually_reset_inventory( + InventoryA0, NodeAAsVirigin), ?LOG_DEBUG( "Feature flags: inventory of node `~ts`:~n~tp", [NodeA, InventoryA], @@ -469,6 +484,34 @@ list_nodes_clustered_with(Node) -> ListOrError -> ListOrError end. +virtually_reset_inventory( + #{feature_flags := FeatureFlags, + states_per_node := StatesPerNode} = Inventory, + true = _NodeAsVirgin) -> + [Node | _] = maps:keys(StatesPerNode), + FeatureStates0 = maps:get(Node, StatesPerNode), + FeatureStates1 = maps:map( + fun(FeatureName, _FeatureState) -> + FeatureProps = maps:get( + FeatureName, FeatureFlags), + Stability = rabbit_feature_flags:get_stability( + FeatureProps), + case Stability of + required -> true; + _ -> false + end + end, FeatureStates0), + StatesPerNode1 = maps:map( + fun(_Node, _FeatureStates) -> + FeatureStates1 + end, StatesPerNode), + Inventory1 = Inventory#{states_per_node => StatesPerNode1}, + Inventory1; +virtually_reset_inventory( + Inventory, + false = _NodeAsVirgin) -> + Inventory. + -spec are_compatible(Inventory, Inventory) -> AreCompatible when Inventory :: rabbit_feature_flags:cluster_inventory(), AreCompatible :: boolean().