Merge pull request #7156 from rabbitmq/fix-default-feature-flags-filtering-in-ff-controller

Feature flags: Fix experimental feature flags inventory

Author: dumbbell

deps/rabbit/src/rabbit_feature_flags.erl

@@ -682,7 +682,13 @@ get_state(FeatureName) when is_atom(FeatureName) ->
                  end
     end.
 
--spec get_stability(feature_name() | feature_props_extended()) -> stability().
+-spec get_stability
+(FeatureName) -> Stability | undefined when
+      FeatureName :: feature_name(),
+      Stability :: stability();
+(FeatureProps) -> Stability when
+      FeatureProps :: feature_props_extended(),
+      Stability :: stability().
 %% @doc
 %% Returns the stability of a feature flag.
 %%
@@ -696,12 +702,12 @@ get_state(FeatureName) when is_atom(FeatureName) ->
 %% <li>`experimental': the feature flag is experimental and may change in
 %%   the future (without a guaranteed upgrade path): enabling it in
 %%   production is not recommended.</li>
-%% <li>`unavailable': the feature flag is unsupported by at least one
-%%   node in the cluster and can not be enabled for now.</li>
 %% </ul>
 %%
 %% @param FeatureName The name of the feature flag to check.
-%% @returns `stable' or `experimental'.
+%% @param FeatureProps A feature flag properties map.
+%% @returns `required', `stable' or `experimental', or `undefined' if the
+%% given feature flag name doesn't correspond to a known feature flag.
 
 get_stability(FeatureName) when is_atom(FeatureName) ->
     case rabbit_ff_registry:get(FeatureName) of

deps/rabbit/src/rabbit_ff_controller.erl

@@ -982,12 +982,32 @@ merge_feature_flags(FeatureFlagsA, FeatureFlagsB) ->
     FeatureFlags = maps:merge(FeatureFlagsA, FeatureFlagsB),
     maps:map(
       fun(FeatureName, FeatureProps) ->
-              FeaturePropsA = maps:get(FeatureName, FeatureFlagsA, #{}),
-              FeaturePropsB = maps:get(FeatureName, FeatureFlagsB, #{}),
+              %% When we collect feature flag properties from all nodes, we
+              %% start with an empty cluster inventory (a common Erlang
+              %% recursion pattern). This means that all feature flags are
+              %% unknown at first.
+              %%
+              %% Here we must compute a global stability level for each
+              %% feature flag, in case all nodes are not on the same page
+              %% (like one nodes considers a feature flag experimental, but
+              %% another one marks it as stable). That's why we rank stability
+              %% level: required > stable > experimental.
+              %%
+              %% We must distinguish an unknown feature flag (they are all
+              %% unknown at the beginning of the collection) from a known
+              %% feature flags with no explicit stability level. In the latter
+              %% case, `rabbit_feature_flags:get_stability/1' will consider it
+              %% stable. However in the former case, we must consider it
+              %% experimental otherwise it would default to be stable would
+              %% superceed an experimental level, even though all nodes agree
+              %% on that level, because `rabbit_feature_flags:get_stability/1'
+              %% would return stable as well.
+              UnknownProps = #{stability => experimental},
+              FeaturePropsA = maps:get(
+                                FeatureName, FeatureFlagsA, UnknownProps),
+              FeaturePropsB = maps:get(
+                                FeatureName, FeatureFlagsB, UnknownProps),
 
-              %% There is a rank between stability levels. If a feature flag
-              %% is required somewhere, it is required globally. Otherwise if
-              %% it is stable somewhere, it is stable globally.
               StabilityA = rabbit_feature_flags:get_stability(FeaturePropsA),
               StabilityB = rabbit_feature_flags:get_stability(FeaturePropsB),
               Stability = case {StabilityA, StabilityB} of

deps/rabbit/src/rabbit_ff_registry_factory.erl

@@ -396,7 +396,7 @@ do_initialize_registry(RegistryVsn,
            "Feature flags:   [~ts] ~ts~n",
            [case maps:get(FeatureName, FeatureStates, false) of
                 true           -> "x";
-                state_changing -> "~~";
+                state_changing -> "~";
                 false          -> " "
             end,
             FeatureName])