From 6a0008b06c2963899a522096c9a4a2a421afdbea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-S=C3=A9bastien=20P=C3=A9dron?= Date: Fri, 4 Oct 2024 10:59:09 +0200 Subject: [PATCH 1/2] rabbit_feature_flags: Accept "+feature1,-feature2" in $RABBITMQ_FEATURE_FLAGS [Why] Before this patch, the $RABBITMQ_FEATURE_FLAGS environment variable took an exhaustive list of feature flags to enable. This list overrode the default of enabling all stable feature flags. It made it inconvenient when a user wanted to enable an experimental feature flag like `khepri_db` while still leaving the default behavior. [How] $RABBITMQ_FEATURE_FLAGS now acceps the following syntax: RABBITMQ_FEATURE_FLAGS=+feature1,-feature2 This will start RabbitMQ with all stable feature flags, plus `feature1`, but without `feature2`. For users setting `forced_feature_flags_on_init` in the config, the corresponding syntax is: {forced_feature_flags_on_init, {rel, [feature1], [feature2]}} --- deps/rabbit/src/rabbit_ff_controller.erl | 167 +++++++++++++++-------- deps/rabbit_common/src/rabbit_env.erl | 14 +- 2 files changed, 116 insertions(+), 65 deletions(-) diff --git a/deps/rabbit/src/rabbit_ff_controller.erl b/deps/rabbit/src/rabbit_ff_controller.erl index 822f38b01e90..d6f11a73c9ab 100644 --- a/deps/rabbit/src/rabbit_ff_controller.erl +++ b/deps/rabbit/src/rabbit_ff_controller.erl @@ -675,48 +675,83 @@ enable_task(FeatureNames) -> end. enable_default_task() -> - FeatureNames = get_forced_feature_flag_names(), - case FeatureNames of - undefined -> + case get_forced_feature_flag_names() of + {ok, undefined} -> ?LOG_DEBUG( "Feature flags: starting an unclustered node for the first " "time: all stable feature flags will be enabled by default", #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), {ok, Inventory} = collect_inventory_on_nodes([node()]), - #{feature_flags := FeatureFlags} = Inventory, - StableFeatureNames = - maps:fold( - fun(FeatureName, FeatureProps, Acc) -> - Stability = rabbit_feature_flags:get_stability( - FeatureProps), - case Stability of - stable -> [FeatureName | Acc]; - _ -> Acc - end - end, [], FeatureFlags), + StableFeatureNames = get_stable_feature_flags(Inventory), enable_many(Inventory, StableFeatureNames); - [] -> + {ok, []} -> ?LOG_DEBUG( "Feature flags: starting an unclustered node for the first " "time: all feature flags are forcibly left disabled from " "the $RABBITMQ_FEATURE_FLAGS environment variable", #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), ok; - _ -> + {ok, FeatureNames} when is_list(FeatureNames) -> ?LOG_DEBUG( "Feature flags: starting an unclustered node for the first " "time: only the following feature flags specified in the " "$RABBITMQ_FEATURE_FLAGS environment variable will be enabled: " - "~tp", + "~0tp", [FeatureNames], #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), {ok, Inventory} = collect_inventory_on_nodes([node()]), - enable_many(Inventory, FeatureNames) + enable_many(Inventory, FeatureNames); + {ok, {rel, Plus, Minus}} -> + ?LOG_DEBUG( + "Feature flags: starting an unclustered node for the first " + "time: all stable feature flags will be enabled, after " + "applying changes from $RABBITMQ_FEATURE_FLAGS: adding ~0tp, " + "skipping ~0tp", + [Plus, Minus], + #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), + {ok, Inventory} = collect_inventory_on_nodes([node()]), + StableFeatureNames = get_stable_feature_flags(Inventory), + Unsupported = lists:filter( + fun(FeatureName) -> + not is_known_and_supported( + Inventory, FeatureName) + end, Minus), + case Unsupported of + [] -> + FeatureNames = (StableFeatureNames -- Minus) ++ Plus, + enable_many(Inventory, FeatureNames); + _ -> + ?LOG_ERROR( + "Feature flags: unsupported feature flags to skip in " + "$RABBITMQ_FEATURE_FLAGS: ~0tp", + [Unsupported], + #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), + {error, unsupported} + end; + {error, syntax_error_in_envvar} = Error -> + ?LOG_DEBUG( + "Feature flags: invalid mix of `feature_flag` and " + "`+/-feature_flag` in $RABBITMQ_FEATURE_FLAGS", + #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), + Error end. +get_stable_feature_flags(#{feature_flags := FeatureFlags}) -> + maps:fold( + fun(FeatureName, FeatureProps, Acc) -> + Stability = rabbit_feature_flags:get_stability(FeatureProps), + case Stability of + stable -> [FeatureName | Acc]; + _ -> Acc + end + end, [], FeatureFlags). + -spec get_forced_feature_flag_names() -> Ret when - Ret :: FeatureNames | undefined, - FeatureNames :: [rabbit_feature_flags:feature_name()]. + Ret :: {ok, Abs | Rel | undefined} | {error, syntax_error_in_envvar}, + Abs :: [rabbit_feature_flags:feature_name()], + Rel :: {rel, + [rabbit_feature_flags:feature_name()], + [rabbit_feature_flags:feature_name()]}. %% @doc Returns the (possibly empty) list of feature flags the user wants to %% enable out-of-the-box when starting a node for the first time. %% @@ -737,43 +772,64 @@ enable_default_task() -> %% @private get_forced_feature_flag_names() -> - Ret = case get_forced_feature_flag_names_from_env() of - undefined -> get_forced_feature_flag_names_from_config(); - List -> List - end, - case Ret of - undefined -> - ok; - [] -> - ?LOG_INFO( - "Feature flags: automatic enablement of feature flags " - "disabled (i.e. none will be enabled automatically)", - #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}); - _ -> - ?LOG_INFO( - "Feature flags: automatic enablement of feature flags " - "limited to the following list: ~tp", - [Ret], - #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}) - end, - Ret. + case get_forced_feature_flag_names_from_env() of + {ok, undefined} -> get_forced_feature_flag_names_from_config(); + {ok, _} = Ret -> Ret; + {error, _} = Error -> Error + end. -spec get_forced_feature_flag_names_from_env() -> Ret when - Ret :: FeatureNames | undefined, - FeatureNames :: [rabbit_feature_flags:feature_name()]. + Ret :: {ok, Abs | Rel | undefined} | {error, syntax_error_in_envvar}, + Abs :: [rabbit_feature_flags:feature_name()], + Rel :: {rel, + [rabbit_feature_flags:feature_name()], + [rabbit_feature_flags:feature_name()]}. %% @private get_forced_feature_flag_names_from_env() -> - case rabbit_prelaunch:get_context() of - #{forced_feature_flags_on_init := ForcedFFs} - when is_list(ForcedFFs) -> - ForcedFFs; - _ -> - undefined + Value = case rabbit_prelaunch:get_context() of + #{forced_feature_flags_on_init := ForcedFFs} -> ForcedFFs; + _ -> undefined + end, + case Value of + undefined -> + {ok, Value}; + [] -> + {ok, Value}; + [[Op | _] | _] when Op =:= $+ orelse Op =:= $- -> + lists:foldr( + fun + ([$+ | NameS], {ok, {rel, Plus, Minus}}) -> + Name = list_to_atom(NameS), + Plus1 = [Name | Plus], + {ok, {rel, Plus1, Minus}}; + ([$- | NameS], {ok, {rel, Plus, Minus}}) -> + Name = list_to_atom(NameS), + Minus1 = [Name | Minus], + {ok, {rel, Plus, Minus1}}; + (_, {error, _} = Error) -> + Error; + (_, _) -> + {error, syntax_error_in_envvar} + end, {ok, {rel, [], []}}, Value); + _ when is_list(Value) -> + lists:foldr( + fun + (Name, {ok, Abs}) when is_atom(Name) -> + {ok, [Name | Abs]}; + ([C | _] = NameS, {ok, Abs}) + when C =/= $+ andalso C =/= $- -> + Name = list_to_atom(NameS), + {ok, [Name | Abs]}; + (_, {error, _} = Error) -> + Error; + (_, _) -> + {error, syntax_error_in_envvar} + end, {ok, []}, Value) end. -spec get_forced_feature_flag_names_from_config() -> Ret when - Ret :: FeatureNames | undefined, + Ret :: {ok, FeatureNames | undefined}, FeatureNames :: [rabbit_feature_flags:feature_name()]. %% @private @@ -781,15 +837,8 @@ get_forced_feature_flag_names_from_config() -> Value = application:get_env( rabbit, forced_feature_flags_on_init, undefined), case Value of - undefined -> - Value; - _ when is_list(Value) -> - case lists:all(fun is_atom/1, Value) of - true -> Value; - false -> undefined - end; - _ -> - undefined + undefined -> {ok, Value}; + _ when is_list(Value) -> {ok, Value} end. -spec sync_cluster_task() -> Ret when @@ -914,7 +963,7 @@ enable_if_supported(#{states_per_node := _} = Inventory, FeatureName) -> #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), enable_with_registry_locked(Inventory, FeatureName); false -> - ?LOG_DEBUG( + ?LOG_ERROR( "Feature flags: `~ts`: unsupported; aborting", [FeatureName], #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}), diff --git a/deps/rabbit_common/src/rabbit_env.erl b/deps/rabbit_common/src/rabbit_env.erl index 4f222ab707f4..237a96689626 100644 --- a/deps/rabbit_common/src/rabbit_env.erl +++ b/deps/rabbit_common/src/rabbit_env.erl @@ -999,13 +999,15 @@ forced_feature_flags_on_init(Context) -> case Value of false -> %% get_prefixed_env_var() considers an empty string - %% is the same as an undefined environment variable. - update_context(Context, - forced_feature_flags_on_init, undefined, default); + %% as an undefined environment variable. + update_context( + Context, + forced_feature_flags_on_init, undefined, default); _ -> - Flags = [list_to_atom(V) || V <- string:lexemes(Value, ",")], - update_context(Context, - forced_feature_flags_on_init, Flags, environment) + FeatureNames = string:lexemes(Value, ","), + update_context( + Context, + forced_feature_flags_on_init, FeatureNames, environment) end. log_feature_flags_registry(Context) -> From 9b2c6d95f88ca139e62577775061529c287fa861 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-S=C3=A9bastien=20P=C3=A9dron?= Date: Fri, 4 Oct 2024 11:09:00 +0200 Subject: [PATCH 2/2] rabbit_env: Drop $RABBITMQ_LOG_FF_REGISTRY [Why] Its use was removed when the registry was converted from a compiled module to a persistent_term. --- deps/rabbit_common/src/rabbit_env.erl | 13 ------------- deps/rabbit_common/test/rabbit_env_SUITE.erl | 7 ------- 2 files changed, 20 deletions(-) diff --git a/deps/rabbit_common/src/rabbit_env.erl b/deps/rabbit_common/src/rabbit_env.erl index 237a96689626..16a9bbf22a84 100644 --- a/deps/rabbit_common/src/rabbit_env.erl +++ b/deps/rabbit_common/src/rabbit_env.erl @@ -65,7 +65,6 @@ "RABBITMQ_KEEP_PID_FILE_ON_EXIT", "RABBITMQ_LOG", "RABBITMQ_LOG_BASE", - "RABBITMQ_LOG_FF_REGISTRY", "RABBITMQ_LOGS", "RABBITMQ_MNESIA_BASE", "RABBITMQ_MNESIA_DIR", @@ -150,7 +149,6 @@ get_context_after_reloading_env(Context) -> fun keep_pid_file_on_exit/1, fun feature_flags_file/1, fun forced_feature_flags_on_init/1, - fun log_feature_flags_registry/1, fun plugins_path/1, fun plugins_expand_dir/1, fun enabled_plugins_file/1, @@ -1010,17 +1008,6 @@ forced_feature_flags_on_init(Context) -> forced_feature_flags_on_init, FeatureNames, environment) end. -log_feature_flags_registry(Context) -> - case get_prefixed_env_var("RABBITMQ_LOG_FF_REGISTRY") of - false -> - update_context(Context, - log_feature_flags_registry, false, default); - Value -> - Log = value_is_yes(Value), - update_context(Context, - log_feature_flags_registry, Log, environment) - end. - %% ------------------------------------------------------------------- %% %% RABBITMQ_PLUGINS_DIR diff --git a/deps/rabbit_common/test/rabbit_env_SUITE.erl b/deps/rabbit_common/test/rabbit_env_SUITE.erl index 0961a37a1855..e10ad2f6b428 100644 --- a/deps/rabbit_common/test/rabbit_env_SUITE.erl +++ b/deps/rabbit_common/test/rabbit_env_SUITE.erl @@ -187,7 +187,6 @@ check_default_values(_) -> interactive_shell => default, keep_pid_file_on_exit => default, log_base_dir => default, - log_feature_flags_registry => default, log_levels => default, main_config_file => default, main_log_file => default, @@ -231,7 +230,6 @@ check_default_values(_) -> interactive_shell => false, keep_pid_file_on_exit => false, log_base_dir => "/var/log/rabbitmq", - log_feature_flags_registry => false, log_levels => undefined, main_config_file => "/etc/rabbitmq/rabbitmq", main_log_file => "/var/log/rabbitmq/" ++ NodeS ++ ".log", @@ -282,7 +280,6 @@ check_default_values(_) -> interactive_shell => false, keep_pid_file_on_exit => false, log_base_dir => "%APPDATA%/RabbitMQ/log", - log_feature_flags_registry => false, log_levels => undefined, main_config_file => "%APPDATA%/RabbitMQ/rabbitmq", main_log_file => "%APPDATA%/RabbitMQ/log/" ++ NodeS ++ ".log", @@ -408,7 +405,6 @@ check_values_from_reachable_remote_node(Config) -> interactive_shell => default, keep_pid_file_on_exit => default, log_base_dir => default, - log_feature_flags_registry => default, log_levels => default, main_config_file => default, main_log_file => default, @@ -452,7 +448,6 @@ check_values_from_reachable_remote_node(Config) -> interactive_shell => false, keep_pid_file_on_exit => false, log_base_dir => "/var/log/rabbitmq", - log_feature_flags_registry => false, log_levels => undefined, main_config_file => "/etc/rabbitmq/rabbitmq", main_log_file => "/var/log/rabbitmq/" ++ NodeS ++ ".log", @@ -540,7 +535,6 @@ check_values_from_offline_remote_node(_) -> interactive_shell => default, keep_pid_file_on_exit => default, log_base_dir => default, - log_feature_flags_registry => default, log_levels => default, main_config_file => default, main_log_file => default, @@ -584,7 +578,6 @@ check_values_from_offline_remote_node(_) -> interactive_shell => false, keep_pid_file_on_exit => false, log_base_dir => "/var/log/rabbitmq", - log_feature_flags_registry => false, log_levels => undefined, main_config_file => "/etc/rabbitmq/rabbitmq", main_log_file => "/var/log/rabbitmq/" ++ NodeS ++ ".log",