From 73da2a3fbbd7a1cc6b3930dabed3c2df644a9383 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Wed, 30 Apr 2025 09:38:15 +0200 Subject: [PATCH 1/7] Fix DQT in definition export (redundant property) The correct place for the `default_queue_type` property is inside the `metadata` block. However, right now we'd always export the value outside of `metadata` AND only export it inside `metadata`, if it was not `undefined`. This value outside of `metadata` was just misleading: if a user exported the definitins from a fresh node, changed `classic` to `quorum` and imported such modified values, the DQT would still be `classic`, because RMQ looks for the value inside `metadata`. Just to make it more confusing, if the DQT was changed successfully one way or another, the value outside of `metadata` would reflect that (it always shows the correct value, but is ignored on import). --- deps/rabbit/src/rabbit_definitions.erl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/deps/rabbit/src/rabbit_definitions.erl b/deps/rabbit/src/rabbit_definitions.erl index 0f69b3ddf424..257f76232e10 100644 --- a/deps/rabbit/src/rabbit_definitions.erl +++ b/deps/rabbit/src/rabbit_definitions.erl @@ -1081,12 +1081,10 @@ list_vhosts() -> vhost_definition(VHost) -> Name = vhost:get_name(VHost), - DQT = rabbit_queue_type:short_alias_of(rabbit_vhost:default_queue_type(Name)), #{ <<"name">> => Name, <<"limits">> => vhost:get_limits(VHost), - <<"metadata">> => vhost:get_metadata(VHost), - <<"default_queue_type">> => DQT + <<"metadata">> => vhost:get_metadata(VHost) }. list_users() -> From 5eb65f5f72875c655c9d97a052e85c93ef4e92f5 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Wed, 30 Apr 2025 11:03:14 +0200 Subject: [PATCH 2/7] Remove vhost.default_queue_type from HTTP defs export --- deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl index 6acdf9f7097c..4c6bf620b4c9 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl @@ -288,7 +288,7 @@ export_name(_Name) -> true. rw_state() -> [{users, [name, password_hash, hashing_algorithm, tags, limits]}, - {vhosts, [name, description, tags, default_queue_type, metadata]}, + {vhosts, [name, description, tags, metadata]}, {permissions, [user, vhost, configure, write, read]}, {topic_permissions, [user, vhost, exchange, write, read]}, {parameters, [vhost, component, name, value]}, From 3c95bf32e7a7107e48033ccc1cb0ae90775787c3 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Wed, 30 Apr 2025 14:30:00 +0200 Subject: [PATCH 3/7] vhost inherits DQT from node Rather than injecting node-level DQT when exporting definitions, inject it into vhost's metadata when a vhost is created. --- deps/rabbit/src/rabbit_vhost.erl | 1 + deps/rabbit/src/vhost.erl | 3 ++- deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl | 6 +----- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index bb616a684c77..7bea09935477 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -146,6 +146,7 @@ add(VHost, ActingUser) -> rabbit_types:ok_or_error(any()). add(Name, Description, Tags, ActingUser) -> add(Name, #{description => Description, + default_queue_type => rabbit_queue_type:default_alias(), tags => Tags}, ActingUser). -spec add(vhost:name(), vhost:metadata(), rabbit_types:username()) -> diff --git a/deps/rabbit/src/vhost.erl b/deps/rabbit/src/vhost.erl index a16116a3a99e..796f1224204d 100644 --- a/deps/rabbit/src/vhost.erl +++ b/deps/rabbit/src/vhost.erl @@ -215,7 +215,8 @@ disable_protection_from_deletion(VHost) -> -spec new_metadata(binary(), [atom()], rabbit_queue_type:queue_type() | 'undefined') -> metadata(). new_metadata(Description, Tags, undefined) -> #{description => Description, - tags => Tags}; + default_queue_type => rabbit_queue_type:default_alias(), + tags => Tags}; new_metadata(Description, Tags, DefaultQueueType) -> #{description => Description, tags => Tags, diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl index 4c6bf620b4c9..343c46951d10 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl @@ -132,10 +132,7 @@ vhost_definitions(ReqData, VHostName, Context) -> ProductName = rabbit:product_name(), ProductVersion = rabbit:product_version(), - DQT = rabbit_queue_type:short_alias_of(rabbit_vhost:default_queue_type(VHostName)), - %% note: the type changes to a map - VHost1 = rabbit_queue_type:inject_dqt(VHost), - Metadata = maps:get(metadata, VHost1), + Metadata = vhost:get_metadata(VHost), TopLevelDefsAndMetadata = [ {rabbit_version, rabbit_data_coercion:to_binary(Vsn)}, @@ -147,7 +144,6 @@ vhost_definitions(ReqData, VHostName, Context) -> {explanation, rabbit_data_coercion:to_binary(io_lib:format("Definitions of virtual host '~ts'", [VHostName]))}, {metadata, Metadata}, {description, vhost:get_description(VHost)}, - {default_queue_type, DQT}, {limits, vhost:get_limits(VHost)} ], Result = TopLevelDefsAndMetadata ++ retain_whitelisted(Contents), From 0e743b5fe73ff470b1d9dd5b9b94f45a4c3c58e1 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Thu, 1 May 2025 09:16:40 +0200 Subject: [PATCH 4/7] Adjust tests to the new behaviour --- deps/rabbit/test/vhost_SUITE.erl | 7 ++++--- deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/deps/rabbit/test/vhost_SUITE.erl b/deps/rabbit/test/vhost_SUITE.erl index 9a70a11de687..5b807f73b07b 100644 --- a/deps/rabbit/test/vhost_SUITE.erl +++ b/deps/rabbit/test/vhost_SUITE.erl @@ -307,13 +307,14 @@ vhost_update_default_queue_type_undefined(Config) -> VHost = <<"update-default_queue_type-with-undefined-test">>, Description = <<"rmqfpas-105 test vhost">>, Tags = [replicate, private], - DefaultQueueType = quorum, + VhostDefaultQueueType = quorum, + NodeDefaultQueueType = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_queue_type, default_alias, []), Trace = false, ActingUser = <<"acting-user">>, try ?assertMatch(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)), - PutVhostArgs0 = [VHost, Description, Tags, DefaultQueueType, Trace, ActingUser], + PutVhostArgs0 = [VHost, Description, Tags, VhostDefaultQueueType, Trace, ActingUser], ?assertMatch(ok, rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, put_vhost, PutVhostArgs0)), @@ -322,7 +323,7 @@ vhost_update_default_queue_type_undefined(Config) -> rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, put_vhost, PutVhostArgs1)), V = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost, lookup, [VHost]), - ?assertMatch(#{default_queue_type := DefaultQueueType}, vhost:get_metadata(V)) + ?assertMatch(#{default_queue_type := NodeDefaultQueueType}, vhost:get_metadata(V)) after rabbit_ct_broker_helpers:delete_vhost(Config, VHost) end. diff --git a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl index 7cae1e5c484e..a44dd8962dd6 100644 --- a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl +++ b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl @@ -2126,7 +2126,6 @@ definitions_vhost_metadata_test(Config) -> ?assertEqual(#{ name => VHostName, description => Desc, - default_queue_type => DQT, tags => Tags, metadata => Metadata }, VH), From 9d0f01b45bdd268635b76bcf3c88793918970fba Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Thu, 1 May 2025 15:53:17 +0200 Subject: [PATCH 5/7] Add DQT to vhost metadata on recovery Vhosts that currently don't have their own default queue type, now inherit it from the node configuration and store it in their metadata going forward. --- deps/rabbit/src/rabbit_vhost.erl | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index 7bea09935477..b27a321daf6f 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -57,6 +57,38 @@ recover(VHost) -> ok = rabbit_file:ensure_dir(VHostStubFile), ok = file:write_file(VHostStubFile, VHost), ok = ensure_config_file(VHost), + + %% in the past, a vhost didn't necessarily have a default queue type + %% and queues declared in that vhost defaulted to the type configured + %% on the node level (in the config file). Now each vhost has its default + %% queue type in the metadata. For vhosts updated from older versions, + %% we need to add the default type to the metadata + case rabbit_db_vhost:get(VHost) of + undefined -> + rabbit_log:warning("Cannot check metadata for vhost '~ts' during recovery, record not found.", + [VHost]); + VHostRecord -> + Metadata = vhost:get_metadata(VHostRecord), + case maps:is_key(default_queue_type, Metadata) of + true -> + rabbit_log:debug("Default queue type for vhost '~ts' is ~p.", + [VHost, maps:get(default_queue_type, Metadata)]), + ok; + false -> + DefaultType = rabbit_queue_type:default_alias(), + rabbit_log:info("Setting missing default queue type to '~p' for vhost '~ts'.", + [DefaultType, VHost]), + case rabbit_db_vhost:merge_metadata(VHost, #{default_queue_type => DefaultType}) of + {ok, _UpdatedVHostRecord} -> + ok; + {error, Reason} -> + % Log the error but continue recovery + rabbit_log:warning("Failed to set the default queue type for vhost '~ts': ~p", + [VHost, Reason]) + end + end + end, + {Recovered, Failed} = rabbit_amqqueue:recover(VHost), AllQs = Recovered ++ Failed, QNames = [amqqueue:get_name(Q) || Q <- AllQs], From 9bd11b449fb603ebb18ad96e68f1db62ecbd3225 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Fri, 2 May 2025 13:55:54 +0200 Subject: [PATCH 6/7] Set the DQT in rabbit_vhost:do_add --- deps/rabbit/src/rabbit_vhost.erl | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl index b27a321daf6f..9a88d38ee43e 100644 --- a/deps/rabbit/src/rabbit_vhost.erl +++ b/deps/rabbit/src/rabbit_vhost.erl @@ -178,7 +178,6 @@ add(VHost, ActingUser) -> rabbit_types:ok_or_error(any()). add(Name, Description, Tags, ActingUser) -> add(Name, #{description => Description, - default_queue_type => rabbit_queue_type:default_alias(), tags => Tags}, ActingUser). -spec add(vhost:name(), vhost:metadata(), rabbit_types:username()) -> @@ -190,8 +189,16 @@ add(Name, Metadata, ActingUser) -> catch(do_add(Name, Metadata, ActingUser)) end. -do_add(Name, Metadata, ActingUser) -> +do_add(Name, Metadata0, ActingUser) -> ok = is_over_vhost_limit(Name), + + Metadata = case maps:is_key(default_queue_type, Metadata0) of + true -> + Metadata0; + false -> + Metadata0#{default_queue_type => rabbit_queue_type:default_alias()} + end, + Description = maps:get(description, Metadata, undefined), Tags = maps:get(tags, Metadata, []), From f61b9d9bf410bd6faa8e9a2ac4bf537a518a6ad0 Mon Sep 17 00:00:00 2001 From: Michal Kuratczyk Date: Fri, 2 May 2025 14:01:10 +0200 Subject: [PATCH 7/7] Add a test for DQT upon vhost creation --- deps/rabbit/test/vhost_SUITE.erl | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/deps/rabbit/test/vhost_SUITE.erl b/deps/rabbit/test/vhost_SUITE.erl index 5b807f73b07b..35a32a27d3c5 100644 --- a/deps/rabbit/test/vhost_SUITE.erl +++ b/deps/rabbit/test/vhost_SUITE.erl @@ -27,6 +27,7 @@ all() -> groups() -> ClusterSize1Tests = [ vhost_is_created_with_default_limits, + vhost_is_created_with_default_queue_type, vhost_is_created_with_operator_policies, vhost_is_created_with_default_user, single_node_vhost_deletion_forces_connection_closure, @@ -461,10 +462,37 @@ vhost_is_created_with_default_limits(Config) -> ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)), ?assertEqual(Limits, rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_vhost_limit, list, [VHost])) + after + rabbit_ct_broker_helpers:rpc( + Config, 0, + application, unset_env, [rabbit, default_limits]) + end. + +vhost_is_created_with_default_queue_type(Config) -> + VHost = atom_to_binary(?FUNCTION_NAME), + QName = atom_to_binary(?FUNCTION_NAME), + ?assertEqual(ok, rabbit_ct_broker_helpers:rpc(Config, 0, + application, set_env, [rabbit, default_queue_type, rabbit_quorum_queue])), + try + ?assertEqual(ok, rabbit_ct_broker_helpers:add_vhost(Config, VHost)), + rabbit_ct_broker_helpers:set_full_permissions(Config, <<"guest">>, VHost), + ?assertEqual(<<"quorum">>, rabbit_ct_broker_helpers:rpc(Config, 0, + rabbit_vhost, default_queue_type, [VHost])), + V = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_db_vhost, get, [VHost]), + ct:pal("Vhost metadata: ~p", [V]), + ?assertEqual(<<"quorum">>, maps:get(default_queue_type, vhost:get_metadata(V))), + + Conn = rabbit_ct_client_helpers:open_unmanaged_connection(Config, 0, VHost), + {ok, Chan} = amqp_connection:open_channel(Conn), + amqp_channel:call(Chan, #'queue.declare'{queue = QName, durable = true}), + QNameRes = rabbit_misc:r(VHost, queue, QName), + {ok, Q} = rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_amqqueue, lookup, [QNameRes]), + ?assertMatch(rabbit_quorum_queue, amqqueue:get_type(Q)), + close_connections([Conn]) after rabbit_ct_broker_helpers:rpc( Config, 0, - application, unset_env, [rabbit, default_limits]) + application, unset_env, [rabbit, default_queue_type]) end. vhost_is_created_with_operator_policies(Config) ->