From 55f4e675f9ad93f88f3cda77c7a21d860332da59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-S=C3=A9bastien=20P=C3=A9dron?= Date: Fri, 16 Dec 2022 12:00:59 +0100 Subject: [PATCH] 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 | 2 +- deps/rabbit/test/feature_flags_SUITE.erl | 85 ++------- deps/rabbit/test/feature_flags_v2_SUITE.erl | 189 +++----------------- 3 files changed, 41 insertions(+), 235 deletions(-) diff --git a/deps/rabbit/src/rabbit_core_ff.erl b/deps/rabbit/src/rabbit_core_ff.erl index 1a6cafadc7..f1b7c1736c 100644 --- a/deps/rabbit/src/rabbit_core_ff.erl +++ b/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( diff --git a/deps/rabbit/test/feature_flags_SUITE.erl b/deps/rabbit/test/feature_flags_SUITE.erl index 400de624ca..71965bfbc5 100644 --- a/deps/rabbit/test/feature_flags_SUITE.erl +++ b/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 - %% supported, so if this is not a mixed version test, it is assumed - %% to be supported. + %% `feature_flags_v2' is now required and won't work in mixed-version + %% clusters if the other version doesn't support it. 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 - %% testing in that case. + %% in running them. %% 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. - UsingFFv1 = not ?config(enable_feature_flags_v2, Config), - case UsingFFv1 of - true -> - ?assertEqual( - {error, unsupported}, - enable_feature_flag_on(Config, B, FeatureName)); - false -> - ?assertEqual( - {error, missing_clustered_nodes}, - enable_feature_flag_on(Config, B, FeatureName)) - end, + %% `ff_from_testsuite', if the network is broken. + ?assertEqual( + {error, missing_clustered_nodes}, + enable_feature_flag_on(Config, B, FeatureName)), ?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 -> - ?assertEqual([true, true], - is_feature_flag_supported(Config, plugin_ff)), - ?assertEqual([true, true], - is_feature_flag_enabled(Config, plugin_ff)); - true when UsingFFv2 -> + true -> ?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 -> - ?assertEqual([true, true], - is_feature_flag_supported(Config, plugin_ff)), - ?assertEqual([true, true], - is_feature_flag_enabled(Config, plugin_ff)); - true when UsingFFv2 -> + true -> ?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 - ({Node, true}) -> + fun(Node) -> rabbit_ct_broker_helpers:rpc( Config, Node, rabbit_feature_flags, inject_test_feature_flags, - [FeatureFlags]); - ({Node, false}) -> - Attributes = feature_flags_to_app_attrs(FeatureFlags), - rabbit_ct_broker_helpers:rpc( - Config, Node, - rabbit_feature_flags, - inject_test_feature_flags, - [Attributes]) - end, UseFFv2). + [FeatureFlags]) + end, Nodes). %% Convert to the format expected on RabbitMQ up-to 3.10.x. feature_flags_to_app_attrs(FeatureFlags) when is_map(FeatureFlags) -> diff --git a/deps/rabbit/test/feature_flags_v2_SUITE.erl b/deps/rabbit/test/feature_flags_v2_SUITE.erl index e72d9a465f..d682d00fb9 100644 --- a/deps/rabbit/test/feature_flags_v2_SUITE.erl +++ b/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 - 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 = rabbit_feature_flags:enable(feature_flags_v2), 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 - true -> - ?assert( - rabbit_feature_flags:is_supported(FeatureName)), - ?assert( - rabbit_feature_flags:is_enabled(FeatureName)); - false -> - ?assert( - rabbit_feature_flags:is_supported(FeatureName)), - ?assertNot( - rabbit_feature_flags:is_enabled(FeatureName)) - end, + ?assert( + rabbit_feature_flags:is_supported(FeatureName)), + ?assertNot( + rabbit_feature_flags:is_enabled(FeatureName)), 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 - %% function is executed on the node where `enable()' was - %% called, and then on the node joining the cluster. + %% The migration function is executed on the node where + %% `enable()' was called, and then on the node joining the + %% 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 - true -> - %% With v1, the migration function runs on a - %% single node in the cluster only in this - %% scenario. - %% - %% 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, + %% 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. + ExpectedNodes = [FirstNode, NewNode], %% 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 = case UsingFFv1 of - true -> ok; - false -> {error, {badrpc, nodedown}} - end, + ExpectedRet = {error, {badrpc, nodedown}}, 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 " - "cluster"), + "Checking the feature flag is disabled in the cluster"), _ = [ok = run_on_node( Node, fun() -> - case UsingFFv1 of - true -> - ?assert( - rabbit_feature_flags:is_enabled(FeatureName)); - false -> - ?assertNot( - rabbit_feature_flags:is_enabled(FeatureName)) - end, + ?assertNot( + rabbit_feature_flags:is_enabled(FeatureName)), 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 " - "cluster"), + "Checking the feature flag is disabled in the cluster"), _ = [ok = run_on_node( Node, fun() -> - case UsingFFv1 of - true -> - ?assertNot( - rabbit_feature_flags:is_enabled(FeatureName)); - false -> - ?assertNot( - rabbit_feature_flags:is_enabled(FeatureName)) - end, + ?assertNot( + rabbit_feature_flags:is_enabled(FeatureName)), 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, [])