Merge pull request #8946 from rabbitmq/feature-flags-and-rabbit_db-improvements

Improvements to feature flags and `rabbit_db*`

Author: dumbbell

deps/rabbit/src/rabbit_db_cluster.erl

@@ -58,8 +58,7 @@ can_join(RemoteNode) ->
        #{domain => ?RMQLOG_DOMAIN_DB}),
     case rabbit_feature_flags:check_node_compatibility(RemoteNode) of
         ok ->
-            rabbit_db:run(
-              #{mnesia => fun() -> can_join_using_mnesia(RemoteNode) end});
+            can_join_using_mnesia(RemoteNode);
         Error ->
             Error
     end.
@@ -79,16 +78,16 @@ join(RemoteNode, NodeType)
   when is_atom(RemoteNode) andalso ?IS_NODE_TYPE(NodeType) ->
     case can_join(RemoteNode) of
         {ok, ClusterNodes} when is_list(ClusterNodes) ->
-            rabbit_db:reset(),
+            ok = rabbit_db:reset(),
 
             ?LOG_INFO(
                "DB: joining cluster using remote nodes:~n~tp", [ClusterNodes],
                #{domain => ?RMQLOG_DOMAIN_DB}),
-            Ret = rabbit_db:run(
-                    #{mnesia =>
-                      fun() -> join_using_mnesia(ClusterNodes, NodeType) end}),
+            Ret =  join_using_mnesia(ClusterNodes, NodeType),
             case Ret of
                 ok ->
+                    rabbit_feature_flags:copy_feature_states_after_reset(
+                      RemoteNode),
                     rabbit_node_monitor:notify_joined_cluster(),
                     ok;
                 {error, _} = Error ->

deps/rabbit/src/rabbit_feature_flags.erl

@@ -120,6 +120,7 @@
          query_supported_feature_flags/0,
          does_enabled_feature_flags_list_file_exist/0,
          read_enabled_feature_flags_list/0,
+         copy_feature_states_after_reset/1,
          uses_callbacks/1,
          reset_registry/0]).
 
@@ -1122,10 +1123,13 @@ try_to_write_enabled_feature_flags_list(FeatureNames) ->
                                   false -> [Name | Acc]
                               end
                       end, FeatureNames1, PreviouslyEnabled),
-    FeatureNames3 = lists:sort(FeatureNames2),
+    do_write_enabled_feature_flags_list(FeatureNames2).
+
+do_write_enabled_feature_flags_list(EnabledFeatureNames) ->
+    EnabledFeatureNames1 = lists:sort(EnabledFeatureNames),
 
     File = enabled_feature_flags_list_file(),
-    Content = io_lib:format("~tp.~n", [FeatureNames3]),
+    Content = io_lib:format("~tp.~n", [EnabledFeatureNames1]),
     %% TODO: If we fail to write the the file, we should spawn a process
     %% to retry the operation.
     case file:write_file(File, Content) of
@@ -1152,6 +1156,24 @@ enabled_feature_flags_list_file() ->
         undefined -> throw(feature_flags_file_not_set)
     end.
 
+copy_feature_states_after_reset(RemoteNode) ->
+    ?assertEqual(undefined, erlang:whereis(rabbit_ff_controller)),
+    EnabledFeatureFlags = erpc:call(
+                            RemoteNode, ?MODULE, list, [enabled], 60000),
+    EnabledFeatureNames = maps:keys(EnabledFeatureFlags),
+    ?LOG_DEBUG(
+       "Feature flags: copy feature states from remote node `~ts` after "
+       "a reset of this node `~ts`; enabled feature flags:~n~p",
+       [RemoteNode, node(), EnabledFeatureNames],
+       #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}),
+    case do_write_enabled_feature_flags_list(EnabledFeatureNames) of
+        ok ->
+            ok;
+        {error, Reason} ->
+            File = enabled_feature_flags_list_file(),
+            throw({feature_flags_file_copy_error, File, Reason})
+    end.
+
 %% -------------------------------------------------------------------
 %% Feature flags management: enabling.
 %% -------------------------------------------------------------------

deps/rabbit/src/rabbit_ff_registry_factory.erl

@@ -258,7 +258,7 @@ maybe_initialize_registry(NewSupportedFeatureFlags,
                          false ->
                              NewFeatureStates
                      end,
-    FeatureStates =
+    FeatureStates1 =
     maps:map(
       fun
           (FeatureName, FeatureProps) when ?IS_FEATURE_FLAG(FeatureProps) ->
@@ -308,6 +308,18 @@ maybe_initialize_registry(NewSupportedFeatureFlags,
               end
       end, AllFeatureFlags),
 
+    %% We don't record the state of deprecated features because it is
+    %% controlled from configuration and they can be disabled (the deprecated
+    %% feature can be turned back on) if the deprecated feature allows it.
+    %%
+    %% However, some feature flags may depend on deprecated features. If those
+    %% feature flags are enabled, we need to enable the deprecated features
+    %% (turn off the deprecated features) they depend on regardless of the
+    %% configuration.
+    FeatureStates =
+    enable_deprecated_features_required_by_enabled_feature_flags(
+      AllFeatureFlags, FeatureStates1),
+
     %% The feature flags inventory is used by rabbit_ff_controller to query
     %% feature flags atomically. The inventory also contains the list of
     %% scanned applications: this is used to determine if an application is
@@ -400,6 +412,39 @@ does_registry_need_refresh(AllFeatureFlags,
             true
     end.
 
+-spec enable_deprecated_features_required_by_enabled_feature_flags(
+        FeatureFlags, FeatureStates) -> NewFeatureStates when
+      FeatureFlags :: rabbit_feature_flags:feature_flags(),
+      FeatureStates :: rabbit_feature_flags:feature_states(),
+      NewFeatureStates :: rabbit_feature_flags:feature_states().
+
+enable_deprecated_features_required_by_enabled_feature_flags(
+  FeatureFlags, FeatureStates) ->
+    FeatureStates1 =
+    maps:map(
+      fun
+          (DependencyName, false) ->
+              RequiredBy =
+              maps:filter(
+                fun
+                    (FeatureName, #{depends_on := DependsOn}) ->
+                        lists:member(DependencyName, DependsOn) andalso
+                        maps:get(FeatureName, FeatureStates) =:= true;
+                    (_FeatureName, _FeatureProps) ->
+                        false
+                end, FeatureFlags),
+              maps:size(RequiredBy) > 0;
+          (_DependencyName, State) ->
+              State
+      end, FeatureStates),
+    case FeatureStates1 of
+        FeatureStates ->
+            FeatureStates;
+        _ ->
+            enable_deprecated_features_required_by_enabled_feature_flags(
+              FeatureFlags, FeatureStates1)
+    end.
+
 -spec do_initialize_registry(Vsn,
                              FeatureFlags,
                              FeatureStates,

deps/rabbit/test/deprecated_features_SUITE.erl

@@ -35,6 +35,7 @@
          get_appropriate_warning_when_denied/1,
          get_appropriate_warning_when_disconnected/1,
          get_appropriate_warning_when_removed/1,
+         deprecated_feature_enabled_if_feature_flag_depends_on_it/1,
 
          feature_is_unused/1,
          feature_is_used/1
@@ -65,7 +66,8 @@ groups() ->
              get_appropriate_warning_when_permitted,
              get_appropriate_warning_when_denied,
              get_appropriate_warning_when_disconnected,
-             get_appropriate_warning_when_removed
+             get_appropriate_warning_when_removed,
+             deprecated_feature_enabled_if_feature_flag_depends_on_it
             ],
     [
      {cluster_size_1, [], Tests},
@@ -97,10 +99,14 @@ end_per_group(_Group, Config) ->
     Config.
 
 init_per_testcase(Testcase, Config) ->
+    NodesCount = ?config(nodes_count, Config),
+    NodenamePrefix = list_to_atom(
+                       lists:flatten(
+                         io_lib:format("~s-cs~b", [Testcase, NodesCount]))),
     rabbit_ct_helpers:run_steps(
       Config,
       [fun(Cfg) ->
-               feature_flags_v2_SUITE:start_slave_nodes(Cfg, Testcase)
+               feature_flags_v2_SUITE:start_slave_nodes(Cfg, NodenamePrefix)
        end]).
 
 end_per_testcase(_Testcase, Config) ->
@@ -631,3 +637,92 @@ get_appropriate_warning_when_removed(Config) ->
                    ok
            end)
          || Node <- AllNodes].
+
+deprecated_feature_enabled_if_feature_flag_depends_on_it(Config) ->
+    [FirstNode | _] = AllNodes = ?config(nodes, Config),
+    feature_flags_v2_SUITE:connect_nodes(AllNodes),
+    feature_flags_v2_SUITE:override_running_nodes(AllNodes),
+
+    FeatureName = ?FUNCTION_NAME,
+    FeatureFlags = #{FeatureName =>
+                     #{provided_by => rabbit,
+                       deprecation_phase => permitted_by_default},
+
+                     my_feature_flag =>
+                     #{provided_by => rabbit,
+                       stability => experimental,
+                       depends_on => [FeatureName]}},
+    ?assertEqual(
+       ok,
+       feature_flags_v2_SUITE:inject_on_nodes(AllNodes, FeatureFlags)),
+
+    _ = [ok =
+         feature_flags_v2_SUITE:run_on_node(
+           Node,
+           fun() ->
+                   ?assert(rabbit_feature_flags:is_supported(FeatureName)),
+                   ?assertNot(rabbit_feature_flags:is_enabled(FeatureName)),
+                   ?assert(
+                      rabbit_deprecated_features:is_permitted(FeatureName)),
+                   ok
+           end)
+         || Node <- AllNodes],
+
+    ok = feature_flags_v2_SUITE:run_on_node(
+           FirstNode,
+           fun() ->
+                   ?assertEqual(
+                      ok,
+                      rabbit_feature_flags:enable(my_feature_flag)),
+                   ok
+           end),
+
+    _ = [ok =
+         feature_flags_v2_SUITE:run_on_node(
+           Node,
+           fun() ->
+                   ?assert(rabbit_feature_flags:is_enabled(my_feature_flag)),
+
+                   ?assert(rabbit_feature_flags:is_supported(FeatureName)),
+                   ?assert(rabbit_feature_flags:is_enabled(FeatureName)),
+                   ?assertNot(
+                      rabbit_deprecated_features:is_permitted(FeatureName)),
+                   ok
+           end )
+         || Node <- AllNodes],
+
+    _ = [ok =
+         feature_flags_v2_SUITE:run_on_node(
+           Node,
+           fun() ->
+                   ?assertEqual(
+                      ok,
+                      rabbit_ff_registry_factory:reset_registry()),
+                   ok
+           end )
+         || Node <- AllNodes],
+
+    _ = [ok =
+         feature_flags_v2_SUITE:run_on_node(
+           Node,
+           fun() ->
+                   ?assertEqual(
+                      ok,
+                      rabbit_ff_registry_factory:initialize_registry()),
+                   ok
+           end )
+         || Node <- AllNodes],
+
+    _ = [ok =
+         feature_flags_v2_SUITE:run_on_node(
+           Node,
+           fun() ->
+                   ?assert(rabbit_feature_flags:is_enabled(my_feature_flag)),
+
+                   ?assert(rabbit_feature_flags:is_supported(FeatureName)),
+                   ?assert(rabbit_feature_flags:is_enabled(FeatureName)),
+                   ?assertNot(
+                      rabbit_deprecated_features:is_permitted(FeatureName)),
+                   ok
+           end )
+         || Node <- AllNodes].