rabbit_feature_flags: Mark `feature_flags_v2` as required

This means users of RabbitMQ 3.10.x or older will have to upgrade to
RabbitMQ 3.11.x and enable `feature_flags_v2` before they can upgrade to
a more recent release.

deps/rabbit/src/rabbit_core_ff.erl

@@ -70,7 +70,7 @@
 -rabbit_feature_flag(
     {feature_flags_v2,
      #{desc          => "Feature flags subsystem V2",
-       stability     => stable
+       stability     => required
      }}).
 
 -rabbit_feature_flag(

deps/rabbit/test/feature_flags_SUITE.erl

@@ -48,7 +48,6 @@ suite() ->
 all() ->
     [
      {group, registry},
-     {group, feature_flags_v1},
      {group, feature_flags_v2}
     ].
 
@@ -94,7 +93,6 @@ groups() ->
        registry_general_usage,
        registry_concurrent_reloads
       ]},
-     {feature_flags_v1, [], Groups},
      {feature_flags_v2, [], Groups}
     ].
 
@@ -111,12 +109,9 @@ init_per_suite(Config) ->
 end_per_suite(Config) ->
     rabbit_ct_helpers:run_teardown_steps(Config).
 
-init_per_group(feature_flags_v1, Config) ->
-    rabbit_ct_helpers:set_config(Config, {enable_feature_flags_v2, false});
 init_per_group(feature_flags_v2, Config) ->
-    %% The feature_flags_v2 group only exists on branches where it is
+    %% `feature_flags_v2' is now required and won't work in mixed-version
-    %% supported, so if this is not a mixed version test, it is assumed
+    %% clusters if the other version doesn't support it.
-    %% to be supported.
     case rabbit_ct_helpers:is_mixed_versions() of
         false ->
             rabbit_ct_helpers:set_config(
@@ -124,8 +119,7 @@ init_per_group(feature_flags_v2, Config) ->
         true ->
             %% Before we run `feature_flags_v2'-related tests, we must ensure
             %% that both umbrellas support them. Otherwise there is no point
-            %% in running them. The `feature_flags_v1' group already covers
+            %% in running them.
-            %% testing in that case.
             %% To determine that `feature_flags_v2' are supported, we can't
             %% query RabbitMQ which is not started. Therefore, we check if the
             %% source or bytecode of `rabbit_ff_controller' is present.
@@ -197,16 +191,6 @@ end_per_group(_, Config) ->
 init_per_testcase(Testcase, Config) ->
     rabbit_ct_helpers:testcase_started(Config, Testcase),
     TestNumber = rabbit_ct_helpers:testcase_number(Config, ?MODULE, Testcase),
-    UsingFFv2 = rabbit_ct_helpers:get_config(
-                  Config, enable_feature_flags_v2, false),
-    ForcedFFs = case UsingFFv2 of
-                    true  -> [feature_flags_v2];
-                    false -> []
-                end,
-    Suffix = case UsingFFv2 of
-                 false -> rabbit_misc:format("~ts-v1", [Testcase]);
-                 true  -> rabbit_misc:format("~ts-v2", [Testcase])
-             end,
     case ?config(tc_group_properties, Config) of
         [{name, registry} | _] ->
             logger:set_primary_config(level, debug),
@@ -224,15 +208,14 @@ init_per_testcase(Testcase, Config) ->
             ClusterSize = ?config(rmq_nodes_count, Config),
             Config1 = rabbit_ct_helpers:set_config(
                         Config,
-                        [{rmq_nodename_suffix, Suffix},
+                        [{rmq_nodename_suffix, Testcase},
                          {tcp_ports_base, {skip_n_nodes,
                                            TestNumber * ClusterSize}}
                         ]),
             Config2 = rabbit_ct_helpers:merge_app_env(
                         Config1,
                         {rabbit,
-                         [{forced_feature_flags_on_init, ForcedFFs},
+                         [{log, [{file, [{level, debug}]}]}]}),
-                          {log, [{file, [{level, debug}]}]}]}),
             Config3 = rabbit_ct_helpers:run_steps(
                         Config2,
                         rabbit_ct_broker_helpers:setup_steps() ++
@@ -257,7 +240,7 @@ init_per_testcase(Testcase, Config) ->
             ClusterSize = ?config(rmq_nodes_count, Config),
             Config1 = rabbit_ct_helpers:set_config(
                         Config,
-                        [{rmq_nodename_suffix, Suffix},
+                        [{rmq_nodename_suffix, Testcase},
                          {tcp_ports_base, {skip_n_nodes,
                                            TestNumber * ClusterSize}},
                          {net_ticktime, 5}
@@ -265,8 +248,7 @@ init_per_testcase(Testcase, Config) ->
             Config2 = rabbit_ct_helpers:merge_app_env(
                         Config1,
                         {rabbit,
-                         [{forced_feature_flags_on_init, ForcedFFs},
+                         [{log, [{file, [{level, debug}]}]}]}),
-                          {log, [{file, [{level, debug}]}]}]}),
             Config3 = rabbit_ct_helpers:run_steps(
                         Config2,
                         rabbit_ct_broker_helpers:setup_steps() ++
@@ -699,18 +681,10 @@ enable_feature_flag_with_a_network_partition(Config) ->
     timer:sleep(1000),
 
     %% Enabling the feature flag should fail in the specific case of
-    %% `ff_from_testsuite`, if the network is broken.
+    %% `ff_from_testsuite', if the network is broken.
-    UsingFFv1 = not ?config(enable_feature_flags_v2, Config),
+    ?assertEqual(
-    case UsingFFv1 of
+       {error, missing_clustered_nodes},
-        true ->
+       enable_feature_flag_on(Config, B, FeatureName)),
-            ?assertEqual(
-               {error, unsupported},
-               enable_feature_flag_on(Config, B, FeatureName));
-        false ->
-            ?assertEqual(
-               {error, missing_clustered_nodes},
-               enable_feature_flag_on(Config, B, FeatureName))
-    end,
     ?assertEqual(
        False,
        is_feature_flag_enabled(Config, FeatureName)),
@@ -1005,15 +979,8 @@ clustering_ok_with_new_ff_enabled_from_plugin_on_some_nodes(Config) ->
     ?assertEqual(Config, rabbit_ct_broker_helpers:cluster_nodes(Config)),
 
     log_feature_flags_of_all_nodes(Config),
-    UsingFFv2 = ?config(enable_feature_flags_v2, Config),
-    UsingFFv1 = not UsingFFv2,
     case FFSubsysOk of
-        true when UsingFFv1 ->
+        true ->
-            ?assertEqual([true, true],
-                         is_feature_flag_supported(Config, plugin_ff)),
-            ?assertEqual([true, true],
-                         is_feature_flag_enabled(Config, plugin_ff));
-        true when UsingFFv2 ->
             ?assertEqual([true, true],
                          is_feature_flag_supported(Config, plugin_ff)),
             ?assertEqual([true, false],
@@ -1149,15 +1116,8 @@ activating_plugin_with_new_ff_enabled(Config) ->
     enable_feature_flag_on(Config, 0, plugin_ff),
 
     log_feature_flags_of_all_nodes(Config),
-    UsingFFv2 = ?config(enable_feature_flags_v2, Config),
-    UsingFFv1 = not UsingFFv2,
     case FFSubsysOk of
-        true when UsingFFv1 ->
+        true ->
-            ?assertEqual([true, true],
-                         is_feature_flag_supported(Config, plugin_ff)),
-            ?assertEqual([true, true],
-                         is_feature_flag_enabled(Config, plugin_ff));
-        true when UsingFFv2 ->
             ?assertEqual([true, true],
                          is_feature_flag_supported(Config, plugin_ff)),
             ?assertEqual([true, false],
@@ -1294,27 +1254,14 @@ inject_ff_on_nodes(Config, FeatureFlags) ->
 
 inject_ff_on_nodes(Config, Nodes, FeatureFlags)
   when is_list(Nodes) andalso is_map(FeatureFlags) ->
-    UseFFv2_0 = rabbit_ct_broker_helpers:rpc(
-                Config, Nodes,
-                rabbit_feature_flags, is_supported_locally,
-                [feature_flags_v2]),
-    UseFFv2 = lists:zip(Nodes, UseFFv2_0),
     lists:map(
-      fun
+      fun(Node) ->
-          ({Node, true}) ->
               rabbit_ct_broker_helpers:rpc(
                 Config, Node,
                 rabbit_feature_flags,
                 inject_test_feature_flags,
-                [FeatureFlags]);
+                [FeatureFlags])
-          ({Node, false}) ->
+      end, Nodes).
-              Attributes = feature_flags_to_app_attrs(FeatureFlags),
-              rabbit_ct_broker_helpers:rpc(
-                Config, Node,
-                rabbit_feature_flags,
-                inject_test_feature_flags,
-                [Attributes])
-      end, UseFFv2).
 
 %% Convert to the format expected on RabbitMQ up-to 3.10.x.
 feature_flags_to_app_attrs(FeatureFlags) when is_map(FeatureFlags) ->

deps/rabbit/test/feature_flags_v2_SUITE.erl

@@ -49,7 +49,6 @@ suite() ->
 
 all() ->
     [
-     {group, feature_flags_v1},
      {group, feature_flags_v2}
     ].
 
@@ -78,7 +77,6 @@ groups() ->
       ]}
     ],
     [
-     {feature_flags_v1, [], Groups},
      {feature_flags_v2, [], Groups}
     ].
 
@@ -96,8 +94,6 @@ init_per_suite(Config) ->
 end_per_suite(Config) ->
     Config.
 
-init_per_group(feature_flags_v1, Config) ->
-    rabbit_ct_helpers:set_config(Config, {enable_feature_flags_v2, false});
 init_per_group(feature_flags_v2, Config) ->
     rabbit_ct_helpers:set_config(Config, {enable_feature_flags_v2, true});
 init_per_group(cluster_size_1, Config) ->
@@ -137,19 +133,15 @@ start_slave_nodes(Config, Testcase) ->
     rabbit_ct_helpers:set_config(Config, {nodes, Nodes}).
 
 start_slave_node(Parent, Config, Testcase, N) ->
-    Prefix = case ?config(enable_feature_flags_v2, Config) of
-                 false -> "ffv1";
-                 true  -> "ffv2"
-             end,
     Name = list_to_atom(
-             rabbit_misc:format("~ts-~ts-~b", [Prefix, Testcase, N])),
+             rabbit_misc:format("~ts-~b", [Testcase, N])),
     ct:pal("- Starting slave node `~ts@...`", [Name]),
     {ok, Node} = slave:start(net_adm:localhost(), Name),
     ct:pal("- Slave node `~ts` started", [Node]),
 
     TestCodePath = filename:dirname(code:which(?MODULE)),
     true = rpc:call(Node, code, add_path, [TestCodePath]),
-    ok = run_on_node(Node, fun setup_slave_node/2, [Config, Testcase]),
+    ok = run_on_node(Node, fun setup_slave_node/1, [Config]),
     ct:pal("- Slave node `~ts` configured", [Node]),
     Parent ! {node, self(), Node}.
 
@@ -181,23 +173,12 @@ run_on_node(Node, Fun, Args) ->
 %% Slave node configuration.
 %% -------------------------------------------------------------------
 
-setup_slave_node(Config, Testcase) ->
+setup_slave_node(Config) ->
     ok = setup_logger(),
     ok = setup_data_dir(Config),
     ok = setup_feature_flags_file(Config),
     ok = start_controller(),
-    case Testcase of
+    ok = rabbit_feature_flags:enable(feature_flags_v2),
-        have_required_feature_flag_in_cluster_and_add_member_with_it_disabled
-        ->
-            %% This testcase handles the `feature_flags_v2' state itself.
-            ok;
-        have_required_feature_flag_in_cluster_and_add_member_without_it
-        ->
-            %% This testcase handles the `feature_flags_v2' state itself.
-            ok;
-        _ ->
-            ok = maybe_enable_feature_flags_v2(Config)
-    end,
     ok.
 
 setup_logger() ->
@@ -241,17 +222,6 @@ start_controller() ->
     ?LOG_INFO("Feature flags controller: ~tp", [Pid]),
     ok.
 
-maybe_enable_feature_flags_v2(Config) ->
-    EnableFFv2 = ?config(enable_feature_flags_v2, Config),
-    case EnableFFv2 of
-        true  -> ok = rabbit_feature_flags:enable(feature_flags_v2);
-        false -> ok
-    end,
-    IsEnabled = rabbit_feature_flags:is_enabled(feature_flags_v2),
-    ?LOG_INFO("`feature_flags_v2` enabled: ~ts", [IsEnabled]),
-    ?assertEqual(EnableFFv2, IsEnabled),
-    ok.
-
 override_running_nodes(Nodes) when is_list(Nodes) ->
     ct:pal("Overriding (running) remote nodes for ~tp", [Nodes]),
     _ = [begin
@@ -495,7 +465,6 @@ enable_partially_supported_feature_flag_in_a_3node_cluster(Config) ->
     [FirstNode | OtherNodes] = Nodes = ?config(nodes, Config),
     connect_nodes(Nodes),
     override_running_nodes(Nodes),
-    UsingFFv1 = not ?config(enable_feature_flags_v2, Config),
 
     %% This time, we inject the feature flag on a single node only. The other
     %% nodes don't know about it.
@@ -504,28 +473,6 @@ enable_partially_supported_feature_flag_in_a_3node_cluster(Config) ->
                                       stability => stable}},
     inject_on_nodes([FirstNode], FeatureFlags),
 
-    case UsingFFv1 of
-        true ->
-            %% With `feature_flags_v1', the code would have shared the new
-            %% feature flags with remote nodes, so let's run that here. In the
-            %% end, the testcase is similar to
-            %% `enable_supported_feature_flag_in_a_3node_cluster'.
-            ct:pal("Refreshing feature flags after app load"),
-            ok = run_on_node(
-                   FirstNode,
-                   fun() ->
-                           ?assertEqual(
-                              ok,
-                              rabbit_feature_flags:
-                              share_new_feature_flags_after_app_load(
-                                FeatureFlags, infinity)),
-                           ok
-                   end,
-                   []);
-        false ->
-            ok
-    end,
-
     ct:pal(
       "Checking the feature flag is supported but disabled on all nodes"),
     _ = [ok =
@@ -555,7 +502,7 @@ enable_partially_supported_feature_flag_in_a_3node_cluster(Config) ->
          || Node <- Nodes],
     ct:pal(
       "Checking the feature flag is supported on all nodes and enabled on "
-      "all nodes (v1) or the node knowing it only (v2)"),
+      "the node knowing it only"),
     ok = run_on_node(
            FirstNode,
            fun() ->
@@ -568,18 +515,10 @@ enable_partially_supported_feature_flag_in_a_3node_cluster(Config) ->
          run_on_node(
            Node,
            fun() ->
-                   case UsingFFv1 of
+                   ?assert(
-                       true ->
+                      rabbit_feature_flags:is_supported(FeatureName)),
-                           ?assert(
+                   ?assertNot(
-                              rabbit_feature_flags:is_supported(FeatureName)),
+                      rabbit_feature_flags:is_enabled(FeatureName)),
-                           ?assert(
-                              rabbit_feature_flags:is_enabled(FeatureName));
-                       false ->
-                           ?assert(
-                              rabbit_feature_flags:is_supported(FeatureName)),
-                           ?assertNot(
-                              rabbit_feature_flags:is_enabled(FeatureName))
-                   end,
                    ok
            end,
            [])
@@ -732,9 +671,9 @@ enable_feature_flag_in_cluster_and_add_member_after(Config) ->
            Node,
            fun() ->
                    ?assert(rabbit_feature_flags:is_enabled(FeatureName)),
-                   %% With both feature flags v1 and v2, the migration
+                   %% The migration function is executed on the node where
-                   %% function is executed on the node where `enable()' was
+                   %% `enable()' was called, and then on the node joining the
-                   %% called, and then on the node joining the cluster.
+                   %% cluster.
                    Count = case Node of
                                FirstNode -> 1;
                                NewNode   -> 1;
@@ -873,33 +812,15 @@ enable_feature_flag_in_cluster_and_add_member_concurrently_mfv1(Config) ->
            []),
 
     %% Unblock the migration functions on `Nodes'.
-    UsingFFv1 = not ?config(enable_feature_flags_v2, Config),
     EnablerMRef = erlang:monitor(process, Enabler),
     SyncerMRef = erlang:monitor(process, Syncer),
     unlink(Enabler),
     unlink(Syncer),
-    ExpectedNodes = case UsingFFv1 of
+    %% With v2 but still using the old migration function API (taking 3
-                        true ->
+    %% arguments), the migration function is executed on the node where
-                            %% With v1, the migration function runs on a
+    %% `enable()' was called, and then on the node joining the cluster, thanks
-                            %% single node in the cluster only in this
+    %% to the synchronization.
-                            %% scenario.
+    ExpectedNodes = [FirstNode, NewNode],
-                            %%
-                            %% The reason is that the new node joined during
-                            %% the migration and the feature flag was marked
-                            %% as enabled there as well, even though the
-                            %% migration function possibly didn't know about
-                            %% it. This is one of the problems
-                            %% `feature_flags_v2' fixes.
-                            [FirstNode];
-                        false ->
-                            %% With v2 but still using the old migration
-                            %% function API (taking 3 arguments), the
-                            %% migration function is executed on the node
-                            %% where `enable()' was called, and then on the
-                            %% node joining the cluster, thanks to the
-                            %% synchronization.
-                            [FirstNode, NewNode]
-                    end,
 
     %% Unblock the migration function for which we already consumed the
     %% `waiting' notification.
@@ -1085,12 +1006,6 @@ enable_feature_flag_in_cluster_and_add_member_concurrently_mfv2(Config) ->
 
     %% The migration function runs on all clustered nodes with v2, including
     %% the one joining the cluster, thanks to the synchronization.
-    %%
-    %% When this testcase runs with feature flags v1, the feature flag we want
-    %% to enable uses the migration function API v2: this implicitly enables
-    %% `feature_flags_v2'. As part of the synchronization, the node still on
-    %% feature flags v1 will try to sync `feature_flags_v2' specificaly first.
-    %% After that, the controller-based sync proceeds.
     ExpectedNodes = Nodes ++ [NewNode],
 
     %% Unblock the migration function for which we already consumed the
@@ -1163,11 +1078,7 @@ enable_feature_flag_in_cluster_and_remove_member_concurrently_mfv1(Config) ->
            [])
          || Node <- AllNodes],
 
-    UsingFFv1 = not ?config(enable_feature_flags_v2, Config),
+    ExpectedRet = {error, {badrpc, nodedown}},
-    ExpectedRet = case UsingFFv1 of
-                      true  -> ok;
-                      false -> {error, {badrpc, nodedown}}
-                  end,
     ct:pal(
       "Enabling the feature flag in the cluster (in a separate process)"),
     Peer = self(),
@@ -1224,20 +1135,13 @@ enable_feature_flag_in_cluster_and_remove_member_concurrently_mfv1(Config) ->
     end,
 
     ct:pal(
-      "Checking the feature flag is enabled (v1) or disabled (v2) in the "
+      "Checking the feature flag is disabled in the cluster"),
-      "cluster"),
     _ = [ok =
          run_on_node(
            Node,
            fun() ->
-                   case UsingFFv1 of
+                   ?assertNot(
-                       true ->
+                      rabbit_feature_flags:is_enabled(FeatureName)),
-                           ?assert(
-                              rabbit_feature_flags:is_enabled(FeatureName));
-                       false ->
-                           ?assertNot(
-                              rabbit_feature_flags:is_enabled(FeatureName))
-                   end,
                    ok
            end,
            [])
@@ -1259,8 +1163,6 @@ enable_feature_flag_in_cluster_and_remove_member_concurrently_mfv2(Config) ->
                          {?MODULE, mf_wait_and_count_runs_v2_enable}}}},
     inject_on_nodes(AllNodes, FeatureFlags),
 
-    UsingFFv1 = not ?config(enable_feature_flags_v2, Config),
-
     ct:pal(
       "Checking the feature flag is supported but disabled on all nodes"),
     _ = [ok =
@@ -1323,12 +1225,6 @@ enable_feature_flag_in_cluster_and_remove_member_concurrently_mfv2(Config) ->
     unlink(Enabler),
 
     %% The migration function runs on all clustered nodes with v2.
-    %%
-    %% When this testcase runs with feature flags v1, the feature flag we want
-    %% to enable uses the migration function API v2: this implicitly enables
-    %% `feature_flags_v2'. As part of the synchronization, the node still on
-    %% feature flags v1 will try to sync `feature_flags_v2' specificaly first.
-    %% After that, the controller-based sync proceeds.
     ExpectedNodes = Nodes,
 
     %% Unblock the migration function for which we already consumed the
@@ -1358,20 +1254,13 @@ enable_feature_flag_in_cluster_and_remove_member_concurrently_mfv2(Config) ->
     end,
 
     ct:pal(
-      "Checking the feature flag is enabled (v1) or disabled (v2) in the "
+      "Checking the feature flag is disabled in the cluster"),
-      "cluster"),
     _ = [ok =
          run_on_node(
            Node,
            fun() ->
-                   case UsingFFv1 of
+                   ?assertNot(
-                       true ->
+                      rabbit_feature_flags:is_enabled(FeatureName)),
-                           ?assertNot(
-                              rabbit_feature_flags:is_enabled(FeatureName));
-                       false ->
-                           ?assertNot(
-                              rabbit_feature_flags:is_enabled(FeatureName))
-                   end,
                    ok
            end,
            [])
@@ -1516,12 +1405,6 @@ enable_feature_flag_with_post_enable(Config) ->
 
     %% The migration function runs on all clustered nodes with v2, including
     %% the one joining the cluster, thanks to the synchronization.
-    %%
-    %% When this testcase runs with feature flags v1, the feature flag we want
-    %% to enable uses the migration function API v2: this implicitly enables
-    %% `feature_flags_v2'. As part of the synchronization, the node still on
-    %% feature flags v1 will try to sync `feature_flags_v2' specificaly first.
-    %% After that, the controller-based sync proceeds.
     ExpectedNodes = Nodes ++ [NewNode],
 
     %% Unblock the migration function for which we already consumed the
@@ -1593,8 +1476,6 @@ have_required_feature_flag_in_cluster_and_add_member_with_it_disabled(
            fun() ->
                    ?assert(rabbit_feature_flags:is_supported(FeatureName)),
                    ?assertNot(rabbit_feature_flags:is_enabled(FeatureName)),
-
-                   ok = maybe_enable_feature_flags_v2(Config),
                    ok
            end,
            []),
@@ -1604,16 +1485,6 @@ have_required_feature_flag_in_cluster_and_add_member_with_it_disabled(
            fun() ->
                    ?assert(rabbit_feature_flags:is_supported(FeatureName)),
                    ?assert(rabbit_feature_flags:is_enabled(FeatureName)),
-
-                   %% We enable `feature_flags_v2' on the cluster, regardless
-                   %% of the common_test group. The reason is that we want to
-                   %% test that a bug in the sync between a
-                   %% feature_flags_v2-based cluster and a
-                   %% feature_flags_v1-based joining virgin node is fixed.
-                   %%
-                   %% See:
-                   %% https://github.com/rabbitmq/rabbitmq-server/pull/6791
-                   ok = rabbit_feature_flags:enable(feature_flags_v2),
                    ok
            end,
            [])
@@ -1689,8 +1560,6 @@ have_required_feature_flag_in_cluster_and_add_member_without_it(
                    ?assert(rabbit_feature_flags:is_supported(FeatureName)),
                    ?assertNot(rabbit_feature_flags:is_enabled(FeatureName)),
 
-                   ok = maybe_enable_feature_flags_v2(Config),
-
                    MnesiaDir = rabbit_mnesia:dir(),
                    ok = filelib:ensure_path(MnesiaDir),
                    SomeFile = filename:join(MnesiaDir, "some-mnesia-file.db"),
@@ -1705,16 +1574,6 @@ have_required_feature_flag_in_cluster_and_add_member_without_it(
            fun() ->
                    ?assert(rabbit_feature_flags:is_supported(FeatureName)),
                    ?assert(rabbit_feature_flags:is_enabled(FeatureName)),
-
-                   %% We enable `feature_flags_v2' on the cluster, regardless
-                   %% of the common_test group. The reason is that we want to
-                   %% test that a bug in the sync between a
-                   %% feature_flags_v2-based cluster and a
-                   %% feature_flags_v1-based joining virgin node is fixed.
-                   %%
-                   %% See:
-                   %% https://github.com/rabbitmq/rabbitmq-server/pull/6791
-                   ok = rabbit_feature_flags:enable(feature_flags_v2),
                    ok
            end,
            [])