Merge pull request #7289 from rabbitmq/mergify/bp/v3.10.x/pr-7282

rabbit_vhost:set_tags/2 avoids notifying if tags are unchanged (backport #7280) (backport #7282)

Author: HoloRin

deps/rabbit/BUILD.bazel

@@ -1033,6 +1033,9 @@ rabbitmq_integration_suite(
     name = "vhost_SUITE",
     size = "medium",
     flaky = True,
+    additional_srcs = [
+        "test/test_rabbit_event_handler.erl",
+    ],
 )
 
 rabbitmq_suite(

deps/rabbit/src/rabbit_vhost.erl

@@ -501,33 +501,54 @@ update_metadata(VHostName, Fun) ->
         vhost:set_metadata(Record, Meta)
     end).
 
--spec update_tags(vhost:name(), [vhost_tag()], rabbit_types:username()) -> vhost:vhost() | rabbit_types:ok_or_error(any()).
+are_different0([], []) ->
+    false;
+are_different0([], [_ | _]) ->
+    true;
+are_different0([_ | _], []) ->
+    true;
+are_different0([E], [E]) ->
+    false;
+are_different0([E | R1], [E | R2]) ->
+    are_different0(R1, R2);
+are_different0(_, _) ->
+    true.
+
+are_different(L1, L2) ->
+    are_different0(lists:usort(L1), lists:usort(L2)).
+
+-spec update_tags(vhost:name(), [vhost_tag()], rabbit_types:username()) -> vhost:vhost().
 update_tags(VHostName, Tags, ActingUser) ->
-    ConvertedTags = [rabbit_data_coercion:to_atom(I) || I <- Tags],
+    CurrentTags = case mnesia:dirty_read({rabbit_vhost, VHostName}) of
+        [V] when ?is_vhost(V) -> vhost:get_tags(V);
+        []                    -> []
+    end,
+    ConvertedTags = lists:usort([rabbit_data_coercion:to_atom(I) || I <- Tags]),
     try
         R = rabbit_misc:execute_mnesia_transaction(fun() ->
             update_tags(VHostName, ConvertedTags)
         end),
-        rabbit_log:info("Successfully set tags for virtual host '~s' to ~p", [VHostName, ConvertedTags]),
-        rabbit_event:notify(vhost_tags_set, [{name, VHostName},
-                                             {tags, ConvertedTags},
-                                             {user_who_performed_action, ActingUser}]),
+        rabbit_log:info("Successfully set tags for virtual host '~ts' to ~tp", [VHostName, ConvertedTags]),
+        rabbit_event:notify_if(are_different(CurrentTags, ConvertedTags),
+                               vhost_tags_set, [{name, VHostName},
+                                                {tags, ConvertedTags},
+                                                {user_who_performed_action, ActingUser}]),
         R
     catch
         throw:{error, {no_such_vhost, _}} = Error ->
-            rabbit_log:warning("Failed to set tags for virtual host '~s': the virtual host does not exist", [VHostName]),
+            rabbit_log:warning("Failed to set tags for virtual host '~ts': the virtual host does not exist", [VHostName]),
             throw(Error);
         throw:Error ->
-            rabbit_log:warning("Failed to set tags for virtual host '~s': ~p", [VHostName, Error]),
+            rabbit_log:warning("Failed to set tags for virtual host '~ts': ~tp", [VHostName, Error]),
             throw(Error);
         exit:Error ->
-            rabbit_log:warning("Failed to set tags for virtual host '~s': ~p", [VHostName, Error]),
+            rabbit_log:warning("Failed to set tags for virtual host '~ts': ~tp", [VHostName, Error]),
             exit(Error)
     end.
 
 -spec update_tags(vhost:name(), [vhost_tag()]) -> vhost:vhost() | rabbit_types:ok_or_error(any()).
 update_tags(VHostName, Tags) ->
-    ConvertedTags = [rabbit_data_coercion:to_atom(I) || I <- Tags],
+    ConvertedTags = lists:usort([rabbit_data_coercion:to_atom(I) || I <- Tags]),
     update(VHostName, fun(Record) ->
         Meta0 = vhost:get_metadata(Record),
         Meta  = maps:put(tags, ConvertedTags, Meta0),

deps/rabbit/test/test_rabbit_event_handler.erl

@@ -0,0 +1,33 @@
+-module(test_rabbit_event_handler).
+
+-behaviour(gen_event).
+
+-export([okay/0]).
+-export([init/1, handle_call/2, handle_event/2, handle_info/2,
+    terminate/2, code_change/3]).
+
+-include_lib("rabbit_common/include/rabbit.hrl").
+
+% an exported callable func, used to allow rabbit_ct_broker_helpers
+% to load this code when rpc'd
+okay() -> ok.
+
+init([]) ->
+    {ok, #{events => []}}.
+
+handle_event(#event{} = Event, #{events := Events} = State) ->
+    {ok, State#{events => [Event | Events]}};
+handle_event(_, State) ->
+    {ok, State}.
+
+handle_call(events,  #{events := Events} = State) ->
+    {ok, Events, State}.
+
+handle_info(_Info, State) ->
+    {ok, State}.
+
+terminate(_Arg, _State) ->
+    ok.
+
+code_change(_OldVsn, State, _Extra) ->
+    {ok, State}.

deps/rabbit/test/vhost_SUITE.erl

@@ -26,6 +26,7 @@ groups() ->
         single_node_vhost_deletion_forces_connection_closure,
         vhost_failure_forces_connection_closure,
         vhost_creation_idempotency,
+        vhost_update_idempotency,
         parse_tags
     ],
     ClusterSize2Tests = [
@@ -92,6 +93,13 @@ end_per_group(_Group, Config) ->
       rabbit_ct_client_helpers:teardown_steps() ++
       rabbit_ct_broker_helpers:teardown_steps()).
 
+init_per_testcase(vhost_update_idempotency = Testcase, Config) ->
+    rabbit_ct_helpers:testcase_started(Config, Testcase),
+    clear_all_connection_tracking_tables(Config),
+    case rabbit_ct_broker_helpers:enable_feature_flag(Config, virtual_host_metadata) of
+        ok -> Config;
+        Skip -> Skip
+    end;
 init_per_testcase(Testcase, Config) ->
     rabbit_ct_helpers:testcase_started(Config, Testcase),
     clear_all_connection_tracking_tables(Config),
@@ -319,6 +327,68 @@ vhost_creation_idempotency(Config) ->
         rabbit_ct_broker_helpers:delete_vhost(Config, VHost)
     end.
 
+vhost_update_idempotency(Config) ->
+    VHost = <<"update-idempotency-test">>,
+    ActingUser = <<"acting-user">>,
+    try
+        % load the dummy event handler on the node
+        ok = rabbit_ct_broker_helpers:rpc(Config, 0, test_rabbit_event_handler, okay, []),
+
+        ok = rabbit_ct_broker_helpers:rpc(Config, 0, gen_event, add_handler,
+                                          [rabbit_event, test_rabbit_event_handler, []]),
+
+        ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)),
+
+        ?assertMatch({vhost,VHost, _, #{tags := [private,replicate]}},
+                     rabbit_ct_broker_helpers:rpc(Config, 0,
+                                                  rabbit_vhost, update_tags,
+                                                  [VHost, [private, replicate], ActingUser])),
+        ?assertMatch({vhost,VHost, _, #{tags := [private,replicate]}},
+                     rabbit_ct_broker_helpers:rpc(Config, 0,
+                                                  rabbit_vhost, update_tags,
+                                                  [VHost, [replicate, private], ActingUser])),
+
+        Events = rabbit_ct_broker_helpers:rpc(Config, 0,
+                                              gen_event, call,
+                                              [rabbit_event, test_rabbit_event_handler, events, 100]),
+        ct:pal(?LOW_IMPORTANCE, "Events: ~p", [lists:reverse(Events)]),
+        TagsSetEvents = lists:filter(fun
+                                         (#event{type = vhost_tags_set}) -> true;
+                                         (_) -> false
+                                     end, Events),
+        ?assertMatch([#event{type = vhost_tags_set,
+                             props = [{name, VHost},
+                                      {tags, [private, replicate]},
+                                      {user_who_performed_action, ActingUser}]}],
+                     TagsSetEvents)
+    after
+        rabbit_ct_broker_helpers:rpc(Config, 0,
+                                     gen_event, delete_handler, [rabbit_event, test_rabbit_event_handler, []]),
+        rabbit_ct_broker_helpers:delete_vhost(Config, VHost)
+    end.
+
+vhost_is_created_with_default_limits(Config) ->
+    VHost = <<"vhost1">>,
+    Limits = [{<<"max-connections">>, 10}, {<<"max-queues">>, 1}],
+    Pattern = [{<<"pattern">>, ".*"}],
+    Env = [{vhosts, [{<<"id">>, Limits++Pattern}]}],
+    ?assertEqual(ok, rabbit_ct_broker_helpers:rpc(Config, 0,
+                            application, set_env, [rabbit, default_limits, Env])),
+    ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)),
+    ?assertEqual(Limits, rabbit_ct_broker_helpers:rpc(Config, 0,
+                            rabbit_vhost_limit, list, [VHost])).
+
+vhost_is_created_with_operator_policies(Config) ->
+    VHost = <<"vhost1">>,
+    PolicyName = <<"default-operator-policy">>,
+    Definition = [{<<"expires">>, 10}],
+    Env = [{operator, [{PolicyName, Definition}]}],
+    ?assertEqual(ok, rabbit_ct_broker_helpers:rpc(Config, 0,
+                            application, set_env, [rabbit, default_policies, Env])),
+    ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)),
+    ?assertNotEqual(not_found, rabbit_ct_broker_helpers:rpc(Config, 0,
+                            rabbit_policy, lookup_op, [VHost, PolicyName])).
+
 parse_tags(Config) ->
     rabbit_ct_broker_helpers:rpc(Config, 0, ?MODULE, parse_tags1, [Config]).
 

deps/rabbitmq_cli/test/ctl/set_vhost_tags_command_test.exs

@@ -77,7 +77,7 @@ defmodule SetVhostTagsCommandTest do
         fn record -> record[:vhost] == context[:vhost] end
       )
 
-    assert result[:tags] == context[:tags]
+    assert Enum.sort(result[:tags]) == Enum.sort(context[:tags])
   end
 
   @tag user: @vhost, tags: [:qa]