From 57ed962ef69669cb72c5dd7be93cb7fbf3ca4c0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-S=C3=A9bastien=20P=C3=A9dron?= Date: Wed, 15 Jan 2025 17:29:05 +0100 Subject: [PATCH] rabbitmq_ct_helpers: Fix how we set `$RABBITMQ_FEATURE_FLAGS` in tests [Why] In order to make `khepri_db` the default in the future, the handling of `$RABBITMQ_FEATURE_FLAGS` had to be adapted to be able to *disable* Khepri instead. Unfortunately I broke the behavior with stable feature flags that are only available in the primary umbrella. In this case, they were automatically enabled and thus, clustering with an old umbrella that did not have these feature flags failed with `incompatible_feature_flags`. [How] The solution is to always use an absolute list of feature flags, not the new relative list. V2: Allow a testsuite to skip the configuration of the metadata store. This is needed for the feature_flags_SUITE testsuite because it tests the default behavior and the configuration of the metadata store changes that behavior. While here, fix a ct log message where variables were swapped compared to the format strieg expectation. V3: Enable `rabbitmq_4.0.0` feature flag in rabbit_mgmt_http_SUITE. This testsuite apparently requires it and if it's not enabled, it fails. --- deps/rabbit/test/feature_flags_SUITE.erl | 4 +- .../src/rabbit_ct_broker_helpers.erl | 134 +++++++----------- .../test/rabbit_mgmt_http_SUITE.erl | 12 +- 3 files changed, 66 insertions(+), 84 deletions(-) diff --git a/deps/rabbit/test/feature_flags_SUITE.erl b/deps/rabbit/test/feature_flags_SUITE.erl index c8cc510841ad..027a25f5569e 100644 --- a/deps/rabbit/test/feature_flags_SUITE.erl +++ b/deps/rabbit/test/feature_flags_SUITE.erl @@ -119,7 +119,9 @@ groups() -> init_per_suite(Config) -> rabbit_ct_helpers:log_environment(), - rabbit_ct_helpers:run_setup_steps(Config, [ + Config1 = rabbit_ct_helpers:set_config( + Config, {skip_metadata_store_configuration, true}), + rabbit_ct_helpers:run_setup_steps(Config1, [ fun rabbit_ct_broker_helpers:configure_dist_proxy/1 ]). diff --git a/deps/rabbitmq_ct_helpers/src/rabbit_ct_broker_helpers.erl b/deps/rabbitmq_ct_helpers/src/rabbit_ct_broker_helpers.erl index 296650f96259..f686db6bc4d1 100644 --- a/deps/rabbitmq_ct_helpers/src/rabbit_ct_broker_helpers.erl +++ b/deps/rabbitmq_ct_helpers/src/rabbit_ct_broker_helpers.erl @@ -753,17 +753,6 @@ do_start_rabbitmq_node(Config, NodeConfig, I) -> false -> ExtraArgs3; _ -> ["NOBUILD=1" | ExtraArgs3] end, - %% TODO: When we start to do mixed-version testing against 4.1.x as the - %% secondary umbrella, we will need to stop setting - %% `$RABBITMQ_FEATURE_FLAGS'. - MetadataStore = rabbit_ct_helpers:get_config(Config, metadata_store), - SecFeatureFlags0 = case MetadataStore of - mnesia -> ?REQUIRED_FEATURE_FLAGS; - khepri -> [khepri_db | ?REQUIRED_FEATURE_FLAGS] - end, - SecFeatureFlags = string:join( - [atom_to_list(F) || F <- SecFeatureFlags0], - ","), ExtraArgs = case UseSecondaryUmbrella of true -> DepsDir = ?config(erlang_mk_depsdir, Config), @@ -793,8 +782,7 @@ do_start_rabbitmq_node(Config, NodeConfig, I) -> {"RABBITMQ_SCRIPTS_DIR=~ts", [SecScriptsDir]}, {"RABBITMQ_SERVER=~ts/rabbitmq-server", [SecScriptsDir]}, {"RABBITMQCTL=~ts/rabbitmqctl", [SecScriptsDir]}, - {"RABBITMQ_PLUGINS=~ts/rabbitmq-plugins", [SecScriptsDir]}, - {"RABBITMQ_FEATURE_FLAGS=~ts", [SecFeatureFlags]} + {"RABBITMQ_PLUGINS=~ts/rabbitmq-plugins", [SecScriptsDir]} | ExtraArgs4]; false -> case UseSecondaryDist of @@ -815,8 +803,7 @@ do_start_rabbitmq_node(Config, NodeConfig, I) -> {"CLI_ESCRIPTS_DIR=~ts/escript", [SecondaryDist]}, {"RABBITMQ_SCRIPTS_DIR=~ts/sbin", [SecondaryDist]}, {"RABBITMQ_SERVER=~ts/sbin/rabbitmq-server", [SecondaryDist]}, - {"RABBITMQ_ENABLED_PLUGINS=~ts", [SecondaryEnabledPlugins]}, - {"RABBITMQ_FEATURE_FLAGS=~ts", [SecFeatureFlags]} + {"RABBITMQ_ENABLED_PLUGINS=~ts", [SecondaryEnabledPlugins]} | ExtraArgs4]; false -> ExtraArgs4 @@ -915,19 +902,27 @@ query_node(Config, NodeConfig) -> rabbit_ct_helpers:set_config(NodeConfig, Vars). uses_expected_metadata_store(Config, NodeConfig) -> - %% We want to verify if the active metadata store matches the expected one. - Nodename = ?config(nodename, NodeConfig), - ExpectedMetadataStore = rabbit_ct_helpers:get_config( - Config, metadata_store), - IsKhepriEnabled = rpc(Config, Nodename, rabbit_khepri, is_enabled, []), - UsedMetadataStore = case IsKhepriEnabled of - true -> khepri; - false -> mnesia - end, - ct:pal( - "Metadata store on ~s: expected=~s, used=~s", - [Nodename, UsedMetadataStore, ExpectedMetadataStore]), - {ExpectedMetadataStore, UsedMetadataStore}. + case skip_metadata_store_configuration(Config) of + true -> + {undefined, undefined}; + false -> + %% We want to verify if the active metadata store matches the + %% expected one. + Nodename = ?config(nodename, NodeConfig), + ExpectedMetadataStore = rabbit_ct_helpers:get_config( + Config, metadata_store), + IsKhepriEnabled = rpc( + Config, Nodename, + rabbit_khepri, is_enabled, []), + UsedMetadataStore = case IsKhepriEnabled of + true -> khepri; + false -> mnesia + end, + ct:pal( + "Metadata store on ~s: expected=~s, used=~s", + [Nodename, ExpectedMetadataStore, UsedMetadataStore]), + {ExpectedMetadataStore, UsedMetadataStore} + end. maybe_cluster_nodes(Config) -> Clustered0 = rabbit_ct_helpers:get_config(Config, rmq_nodes_clustered), @@ -1056,62 +1051,37 @@ configured_metadata_store(Config) -> end. configure_metadata_store(Config) -> - ct:log("Configuring metadata store..."), - Value = rabbit_ct_helpers:get_app_env( - Config, rabbit, forced_feature_flags_on_init, undefined), - MetadataStore = configured_metadata_store(Config), - Config1 = rabbit_ct_helpers:set_config( - Config, {metadata_store, MetadataStore}), - %% To enabled or disable `khepri_db', we use the relative forced feature - %% flags mechanism. This allows us to select the state of Khepri without - %% having to worry about other feature flags. - %% - %% However, RabbitMQ 4.0.x and older don't support it. See the - %% `uses_expected_metadata_store/2' check to see how Khepri is enabled in - %% this case. - %% - %% Note that this setting will be ignored by the secondary umbrella because - %% we set `$RABBITMQ_FEATURE_FLAGS' explisitly. In this case, we handle the - %% `khepri_db' feature flag when we compute the value of that variable. - %% - %% TODO: When we start to do mixed-version testing against 4.1.x as the - %% secondary umbrella, we will need to stop setting - %% `$RABBITMQ_FEATURE_FLAGS'. - case MetadataStore of - khepri -> - ct:log("Enabling Khepri metadata store"), - case Value of - undefined -> - rabbit_ct_helpers:merge_app_env( - Config1, - {rabbit, - [{forced_feature_flags_on_init, - {rel, [khepri_db], []}}]}); - _ -> - rabbit_ct_helpers:merge_app_env( - Config1, - {rabbit, - [{forced_feature_flags_on_init, - [khepri_db | Value]}]}) - end; - mnesia -> - ct:log("Enabling Mnesia metadata store"), - case Value of - undefined -> - rabbit_ct_helpers:merge_app_env( - Config1, - {rabbit, - [{forced_feature_flags_on_init, - {rel, [], [khepri_db]}}]}); - _ -> - rabbit_ct_helpers:merge_app_env( - Config1, - {rabbit, - [{forced_feature_flags_on_init, - Value -- [khepri_db]}]}) - end + case skip_metadata_store_configuration(Config) of + true -> + ct:log("Skipping metadata store configuration as requested"), + Config; + false -> + ct:log("Configuring metadata store..."), + MetadataStore = configured_metadata_store(Config), + Config1 = rabbit_ct_helpers:set_config( + Config, {metadata_store, MetadataStore}), + FeatureNames0 = case MetadataStore of + mnesia -> + ct:log("Enabling Mnesia metadata store"), + ?REQUIRED_FEATURE_FLAGS; + khepri -> + ct:log("Enabling Khepri metadata store"), + [khepri_db | ?REQUIRED_FEATURE_FLAGS] + end, + OtherFeatureNames = rabbit_ct_helpers:get_app_env( + Config, + rabbit, forced_feature_flags_on_init, []), + FeatureNames1 = lists:usort(FeatureNames0 ++ OtherFeatureNames), + rabbit_ct_helpers:merge_app_env( + Config1, + {rabbit, [{forced_feature_flags_on_init, FeatureNames1}]}) end. +skip_metadata_store_configuration(Config) -> + Skip = rabbit_ct_helpers:get_config( + Config, skip_metadata_store_configuration), + Skip =:= true. + %% Waits until the metadata store replica on Node is up to date with the leader. await_metadata_store_consistent(Config, Node) -> case configured_metadata_store(Config) of diff --git a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl index 80d77ba8a98a..97a9e3df4e23 100644 --- a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl +++ b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl @@ -244,7 +244,17 @@ start_broker(Config) -> Setup0 = rabbit_ct_broker_helpers:setup_steps(), Setup1 = rabbit_ct_client_helpers:setup_steps(), Steps = Setup0 ++ Setup1, - rabbit_ct_helpers:run_setup_steps(Config, Steps). + case rabbit_ct_helpers:run_setup_steps(Config, Steps) of + {skip, _} = Skip -> + Skip; + Config1 -> + Ret = rabbit_ct_broker_helpers:enable_feature_flag( + Config1, 'rabbitmq_4.0.0'), + case Ret of + ok -> Config1; + _ -> Ret + end + end. finish_init(Group, Config) -> rabbit_ct_helpers:log_environment(),