From 8b8774723bcccee60119c3596bdebe06a3c10f4c Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Fri, 24 May 2024 13:53:58 -0400 Subject: [PATCH 1/2] ctl delete_shovel: use a more effective way Dynamic Shovels keep track of their status in a node-local ETS table which is updated as Shovels go through the lifecycle: start, init (connect, declare the topology), stop. This makes failing Shovels a bit special: their status records will not be long lived, which means it will be considered not to exist by certain code paths. In addition, for such Shovels we do not know what node they are hosted on. But that's fine: we just need to clear their runtime parameter and a periodic Shovel cleanup will remove all children for which no schema database entry (a runtime parameter one) does not exist. rabbitmq_shovel_management's key integration suite has been reworked and expanded to include a case where the Shovel has no chance of successfully connecting. This also deletes a mock-based test suite which does not serve much of a purpose. (cherry picked from commit cae964dba1a1366e64abc32fa725f874e46db7c1) # Conflicts: # deps/rabbitmq_shovel_management/test/http_SUITE.erl # deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_SUITE.erl --- .../src/rabbit_mgmt_test_util.erl | 3 + ...Q.CLI.Ctl.Commands.DeleteShovelCommand.erl | 25 +- ...Q.CLI.Ctl.Commands.ShovelStatusCommand.erl | 2 + .../src/rabbit_shovel_status.erl | 37 ++- .../src/rabbit_shovel_util.erl | 5 + .../src/rabbit_shovel_worker.erl | 3 + deps/rabbitmq_shovel_management/BUILD.bazel | 11 +- deps/rabbitmq_shovel_management/app.bzl | 15 +- .../src/rabbit_shovel_mgmt.erl | 50 ++- .../test/http_SUITE.erl | 308 +++++++++--------- ...vel_mgmt_util_SUITE.erl => unit_SUITE.erl} | 2 +- 11 files changed, 263 insertions(+), 198 deletions(-) rename deps/rabbitmq_shovel_management/test/{rabbit_shovel_mgmt_util_SUITE.erl => unit_SUITE.erl} (98%) diff --git a/deps/rabbitmq_ct_helpers/src/rabbit_mgmt_test_util.erl b/deps/rabbitmq_ct_helpers/src/rabbit_mgmt_test_util.erl index fa6fd2ab3791..3766ab94e6d9 100644 --- a/deps/rabbitmq_ct_helpers/src/rabbit_mgmt_test_util.erl +++ b/deps/rabbitmq_ct_helpers/src/rabbit_mgmt_test_util.erl @@ -175,6 +175,9 @@ http_delete(Config, Path, User, Pass, CodeExp) -> assert_code(CodeExp, CodeAct, "DELETE", Path, ResBody), decode(CodeExp, Headers, ResBody). +http_get_fails(Config, Path) -> + {error, {failed_connect, _}} = req(Config, get, Path, []). + format_for_upload(none) -> <<"">>; format_for_upload(List) -> diff --git a/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.DeleteShovelCommand.erl b/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.DeleteShovelCommand.erl index fcc87743c15a..752de195bded 100644 --- a/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.DeleteShovelCommand.erl +++ b/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.DeleteShovelCommand.erl @@ -69,10 +69,12 @@ run([Name], #{node := Node, vhost := VHost}) -> Error; Xs when is_list(Xs) -> ErrMsg = rabbit_misc:format("Shovel with the given name was not found " - "on the target node '~ts' and / or virtual host '~ts'", + "on the target node '~ts' and/or virtual host '~ts'. " + "It may be failing to connect and report its state, will delete its runtime parameter...", [Node, VHost]), case rabbit_shovel_status:find_matching_shovel(VHost, Name, Xs) of undefined -> + try_force_removing(Node, VHost, Name, ActingUser), {error, rabbit_data_coercion:to_binary(ErrMsg)}; Match -> {{_Name, _VHost}, _Type, {_State, Opts}, _Timestamp} = Match, @@ -83,10 +85,14 @@ run([Name], #{node := Node, vhost := VHost}) -> Error; {error, not_found} -> ErrMsg = rabbit_misc:format("Shovel with the given name was not found " - "on the target node '~ts' and / or virtual host '~ts'", + "on the target node '~ts' and/or virtual host '~ts'. " + "It may be failing to connect and report its state, will delete its runtime parameter...", [Node, VHost]), + try_force_removing(HostingNode, VHost, Name, ActingUser), {error, rabbit_data_coercion:to_binary(ErrMsg)}; - ok -> ok + ok -> + _ = try_clearing_runtime_parameter(Node, VHost, Name, ActingUser), + ok end end end. @@ -99,3 +105,16 @@ aliases() -> output(E, _Opts) -> 'Elixir.RabbitMQ.CLI.DefaultOutput':output(E). + +try_force_removing(Node, VHost, ShovelName, ActingUser) -> + %% Deleting the runtime parameter will cause the dynamic Shovel's child tree to be stopped eventually + %% regardless of the node it is hosted on. MK. + _ = try_clearing_runtime_parameter(Node, VHost, ShovelName, ActingUser), + %% These are best effort attempts to delete the Shovel. Clearing the parameter does all the heavy lifting. MK. + _ = try_stopping_child_process(Node, VHost, ShovelName). + +try_clearing_runtime_parameter(Node, VHost, ShovelName, ActingUser) -> + _ = rabbit_misc:rpc_call(Node, rabbit_runtime_parameters, clear, [VHost, <<"shovel">>, ShovelName, ActingUser]). + +try_stopping_child_process(Node, VHost, ShovelName) -> + _ = rabbit_misc:rpc_call(Node, rabbit_shovel_dyn_worker_sup_sup, stop_and_delete_child, [{VHost, ShovelName}]). diff --git a/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.ShovelStatusCommand.erl b/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.ShovelStatusCommand.erl index add9acdc18f2..9644fd2bdd18 100644 --- a/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.ShovelStatusCommand.erl +++ b/deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.ShovelStatusCommand.erl @@ -112,7 +112,9 @@ fmt_status({'running', Proplist}, Map) -> fmt_status('starting' = St, Map) -> Map#{state => St, source => <<>>, + source_protocol => <<>>, destination => <<>>, + destination_protocol => <<>>, termination_reason => <<>>}; fmt_status({'terminated' = St, Reason}, Map) -> Map#{state => St, diff --git a/deps/rabbitmq_shovel/src/rabbit_shovel_status.erl b/deps/rabbitmq_shovel/src/rabbit_shovel_status.erl index 71a3a03a0094..90ff9cb725f5 100644 --- a/deps/rabbitmq_shovel/src/rabbit_shovel_status.erl +++ b/deps/rabbitmq_shovel/src/rabbit_shovel_status.erl @@ -16,7 +16,9 @@ status/0, lookup/1, cluster_status/0, - cluster_status_with_nodes/0]). + cluster_status_with_nodes/0, + get_status_table/0 +]). -export([inject_node_info/2, find_matching_shovel/3]). -export([init/1, handle_call/3, handle_cast/2, handle_info/2, @@ -93,6 +95,10 @@ cluster_status_with_nodes() -> lookup(Name) -> gen_server:call(?SERVER, {lookup, Name}, infinity). +-spec get_status_table() -> ok. +get_status_table() -> + gen_server:call(?SERVER, get_status_table). + init([]) -> ?ETS_NAME = ets:new(?ETS_NAME, [named_table, {keypos, #entry.name}, private]), @@ -114,11 +120,20 @@ handle_call({lookup, Name}, _From, State) -> {timestamp, Entry#entry.timestamp}]; [] -> not_found end, - {reply, Link, State}. + {reply, Link, State}; + +handle_call(get_status_table, _From, State) -> + Entries = ets:tab2list(?ETS_NAME), + {reply, Entries, State}. handle_cast({report, Name, Type, Info, Timestamp}, State) -> - true = ets:insert(?ETS_NAME, #entry{name = Name, type = Type, info = Info, - timestamp = Timestamp}), + Entry = #entry{ + name = Name, + type = Type, + info = Info, + timestamp = Timestamp + }, + true = ets:insert(?ETS_NAME, Entry), rabbit_event:notify(shovel_worker_status, split_name(Name) ++ split_status(Info)), {noreply, State}; @@ -159,9 +174,17 @@ code_change(_OldVsn, State, _Extra) -> -spec inject_node_info(node(), [status_tuple()]) -> [status_tuple()]. inject_node_info(Node, Shovels) -> lists:map( - fun({Name, Type, {State, Opts}, Timestamp}) -> - Opts1 = Opts ++ [{node, Node}], - {Name, Type, {State, Opts1}, Timestamp} + %% starting + fun({Name, Type, State, Timestamp}) when is_atom(State) -> + Opts = [{node, Node}], + {Name, Type, {State, Opts}, Timestamp}; + %% terminated + ({Name, Type, {terminated, Reason}, Timestamp}) -> + {Name, Type, {terminated, Reason}, Timestamp}; + %% running + ({Name, Type, {State, Opts}, Timestamp}) -> + Opts1 = Opts ++ [{node, Node}], + {Name, Type, {State, Opts1}, Timestamp} end, Shovels). -spec find_matching_shovel(rabbit_types:vhost(), binary(), [status_tuple()]) -> status_tuple() | undefined. diff --git a/deps/rabbitmq_shovel/src/rabbit_shovel_util.erl b/deps/rabbitmq_shovel/src/rabbit_shovel_util.erl index a358199aeacf..c70d0c0e6f3d 100644 --- a/deps/rabbitmq_shovel/src/rabbit_shovel_util.erl +++ b/deps/rabbitmq_shovel/src/rabbit_shovel_util.erl @@ -39,8 +39,13 @@ add_timestamp_header(Props = #'P_basic'{headers = Headers}) -> delete_shovel(VHost, Name, ActingUser) -> case rabbit_shovel_status:lookup({VHost, Name}) of not_found -> + %% Follow the user's obvious intent and delete the runtime parameter just in case the Shovel is in + %% a starting-failing-restarting loop. MK. + rabbit_log:info("Will delete runtime parameters of shovel '~ts' in virtual host '~ts'", [Name, VHost]), + ok = rabbit_runtime_parameters:clear(VHost, <<"shovel">>, Name, ActingUser), {error, not_found}; _Obj -> + rabbit_log:info("Will delete runtime parameters of shovel '~ts' in virtual host '~ts'", [Name, VHost]), ok = rabbit_runtime_parameters:clear(VHost, <<"shovel">>, Name, ActingUser) end. diff --git a/deps/rabbitmq_shovel/src/rabbit_shovel_worker.erl b/deps/rabbitmq_shovel/src/rabbit_shovel_worker.erl index 79d0207f245f..3e5d5c5ec4cb 100644 --- a/deps/rabbitmq_shovel/src/rabbit_shovel_worker.erl +++ b/deps/rabbitmq_shovel/src/rabbit_shovel_worker.erl @@ -62,6 +62,9 @@ handle_call(_Msg, _From, State) -> {noreply, State}. handle_cast(init, State = #state{config = Config0}) -> + rabbit_log_shovel:debug("Shovel ~ts is reporting its status", [human_readable_name(State#state.name)]), + rabbit_shovel_status:report(State#state.name, State#state.type, starting), + rabbit_log_shovel:info("Shovel ~ts will now try to connect...", [human_readable_name(State#state.name)]), try rabbit_shovel_behaviour:connect_source(Config0) of Config -> rabbit_log_shovel:debug("Shovel ~ts connected to source", [human_readable_name(maps:get(name, Config))]), diff --git a/deps/rabbitmq_shovel_management/BUILD.bazel b/deps/rabbitmq_shovel_management/BUILD.bazel index dafb102efb5e..173a83c4fbf9 100644 --- a/deps/rabbitmq_shovel_management/BUILD.bazel +++ b/deps/rabbitmq_shovel_management/BUILD.bazel @@ -100,16 +100,7 @@ rabbitmq_integration_suite( ) rabbitmq_suite( - name = "rabbit_shovel_mgmt_SUITE", - deps = [ - "//deps/rabbit_common:erlang_app", - "//deps/rabbitmq_management_agent:erlang_app", - "@meck//:erlang_app", - ], -) - -rabbitmq_suite( - name = "rabbit_shovel_mgmt_util_SUITE", + name = "unit_SUITE", deps = [ "//deps/rabbit_common:erlang_app", "//deps/rabbitmq_shovel:erlang_app", diff --git a/deps/rabbitmq_shovel_management/app.bzl b/deps/rabbitmq_shovel_management/app.bzl index 0e7a6169cbd4..0ca17b66892d 100644 --- a/deps/rabbitmq_shovel_management/app.bzl +++ b/deps/rabbitmq_shovel_management/app.bzl @@ -99,19 +99,10 @@ def test_suite_beam_files(name = "test_suite_beam_files"): deps = ["//deps/rabbit_common:erlang_app", "//deps/rabbitmq_ct_helpers:erlang_app"], ) erlang_bytecode( - name = "rabbit_shovel_mgmt_SUITE_beam_files", + name = "unit_SUITE_beam_files", testonly = True, - srcs = ["test/rabbit_shovel_mgmt_SUITE.erl"], - outs = ["test/rabbit_shovel_mgmt_SUITE.beam"], - app_name = "rabbitmq_shovel_management", - erlc_opts = "//:test_erlc_opts", - deps = ["//deps/rabbit_common:erlang_app", "//deps/rabbitmq_management_agent:erlang_app"], - ) - erlang_bytecode( - name = "rabbit_shovel_mgmt_util_SUITE_beam_files", - testonly = True, - srcs = ["test/rabbit_shovel_mgmt_util_SUITE.erl"], - outs = ["test/rabbit_shovel_mgmt_util_SUITE.beam"], + srcs = ["test/unit_SUITE.erl"], + outs = ["test/unit_SUITE.beam"], app_name = "rabbitmq_shovel_management", erlc_opts = "//:test_erlc_opts", ) diff --git a/deps/rabbitmq_shovel_management/src/rabbit_shovel_mgmt.erl b/deps/rabbitmq_shovel_management/src/rabbit_shovel_mgmt.erl index 5cd8a6bf9b31..2c414bded340 100644 --- a/deps/rabbitmq_shovel_management/src/rabbit_shovel_mgmt.erl +++ b/deps/rabbitmq_shovel_management/src/rabbit_shovel_mgmt.erl @@ -48,9 +48,15 @@ resource_exists(ReqData, Context) -> %% Deleting or restarting a shovel case get_shovel_node(VHost, Name, ReqData, Context) of undefined -> - rabbit_log:error("Shovel with the name '~ts' was not found on virtual host '~ts'", + rabbit_log:error("Shovel with the name '~ts' was not found on virtual host '~ts'. " + "It may be failing to connect and report its status.", [Name, VHost]), - false; + case is_restart(ReqData) of + true -> false; + %% this is a deletion attempt, it can continue and idempotently try to + %% delete the shovel + false -> true + end; _ -> true end @@ -73,9 +79,17 @@ delete_resource(ReqData, #context{user = #user{username = Username}}=Context) -> Name -> case get_shovel_node(VHost, Name, ReqData, Context) of undefined -> rabbit_log:error("Could not find shovel data for shovel '~ts' in vhost: '~ts'", [Name, VHost]), - false; + case is_restart(ReqData) of + true -> + false; + %% this is a deletion attempt + false -> + %% if we do not know the node, use the local one + try_delete(node(), VHost, Name, Username), + true + end; Node -> - %% We must distinguish between a delete and restart + %% We must distinguish between a delete and a restart case is_restart(ReqData) of true -> rabbit_log:info("Asked to restart shovel '~ts' in vhost '~ts' on node '~s'", [Name, VHost, Node]), @@ -91,17 +105,8 @@ delete_resource(ReqData, #context{user = #user{username = Username}}=Context) -> end; _ -> - rabbit_log:info("Asked to delete shovel '~ts' in vhost '~ts' on node '~s'", [Name, VHost, Node]), - try erpc:call(Node, rabbit_shovel_util, delete_shovel, [VHost, Name, Username], ?SHOVEL_CALLS_TIMEOUT_MS) of - ok -> true; - {error, not_found} -> - rabbit_log:error("Could not find shovel data for shovel '~s' in vhost: '~s'", [Name, VHost]), - false - catch _:Reason -> - rabbit_log:error("Failed to delete shovel '~s' on vhost '~s', reason: ~p", - [Name, VHost, Reason]), - false - end + try_delete(Node, VHost, Name, Username), + true end end @@ -150,3 +155,18 @@ find_matching_shovel(VHost, Name, Shovels) -> _ -> undefined end. + +-spec try_delete(node(), vhost:name(), any(), rabbit_types:username()) -> boolean(). +try_delete(Node, VHost, Name, Username) -> + rabbit_log:info("Asked to delete shovel '~ts' in vhost '~ts' on node '~s'", [Name, VHost, Node]), + %% this will clear the runtime parameter, the ultimate way of deleting a dynamic Shovel eventually. MK. + try erpc:call(Node, rabbit_shovel_util, delete_shovel, [VHost, Name, Username], ?SHOVEL_CALLS_TIMEOUT_MS) of + ok -> true; + {error, not_found} -> + rabbit_log:error("Could not find shovel data for shovel '~s' in vhost: '~s'", [Name, VHost]), + false + catch _:Reason -> + rabbit_log:error("Failed to delete shovel '~s' on vhost '~s', reason: ~p", + [Name, VHost, Reason]), + false + end. diff --git a/deps/rabbitmq_shovel_management/test/http_SUITE.erl b/deps/rabbitmq_shovel_management/test/http_SUITE.erl index 1f80fd319b44..0a053cc66464 100644 --- a/deps/rabbitmq_shovel_management/test/http_SUITE.erl +++ b/deps/rabbitmq_shovel_management/test/http_SUITE.erl @@ -11,60 +11,99 @@ -include_lib("rabbit_common/include/rabbit_framing.hrl"). -include_lib("rabbitmq_ct_helpers/include/rabbit_mgmt_test.hrl"). +-import(rabbit_mgmt_test_util, [http_get/3, http_get/5, http_put/4, http_post/4, http_delete/3, http_delete/4, http_get_fails/2]). +-import(rabbit_ct_helpers, [await_condition/2]). + -compile(export_all). all() -> [ +<<<<<<< HEAD {group, non_parallel_tests} +======= + {group, dynamic_shovels}, + {group, static_shovels}, + {group, plugin_management} +>>>>>>> cae964dba1 (ctl delete_shovel: use a more effective way) ]. groups() -> [ +<<<<<<< HEAD {non_parallel_tests, [], [ amqp10_shovels, shovels, dynamic_plugin_enable_disable ]} +======= + {dynamic_shovels, [], [ + start_and_list_a_dynamic_amqp10_shovel, + create_and_delete_a_dynamic_shovel_that_successfully_connects, + create_and_delete_a_dynamic_shovel_that_fails_to_connect + ]}, + + {static_shovels, [], [ + start_static_shovels + ]}, + + {plugin_management, [], [ + dynamic_plugin_enable_disable + ]} +>>>>>>> cae964dba1 (ctl delete_shovel: use a more effective way) ]. %% ------------------------------------------------------------------- %% Testsuite setup/teardown. %% ------------------------------------------------------------------- -init_per_suite(Config) -> +init_per_group(static_shovels, Config) -> rabbit_ct_helpers:log_environment(), Config1 = rabbit_ct_helpers:set_config(Config, [ {rmq_nodename_suffix, ?MODULE} - ]), + ]), rabbit_ct_helpers:run_setup_steps(Config1, [ fun configure_shovels/1, fun start_inets/1 + ] ++ rabbit_ct_broker_helpers:setup_steps() ++ + rabbit_ct_client_helpers:setup_steps()); +init_per_group(_Group, Config) -> + rabbit_ct_helpers:log_environment(), + Config1 = rabbit_ct_helpers:set_config(Config, [ + {rmq_nodename_suffix, ?MODULE} + ]), + rabbit_ct_helpers:run_setup_steps(Config1, [ + fun start_inets/1 ] ++ rabbit_ct_broker_helpers:setup_steps() ++ rabbit_ct_client_helpers:setup_steps()). -end_per_suite(Config) -> - rabbit_ct_helpers:run_teardown_steps(Config, - rabbit_ct_client_helpers:teardown_steps() ++ - rabbit_ct_broker_helpers:teardown_steps()). +end_per_group(start_static_shovels, Config) -> + http_delete(Config, "/vhosts/v", ?NO_CONTENT), + http_delete(Config, "/users/admin", ?NO_CONTENT), + http_delete(Config, "/users/mon", ?NO_CONTENT), -init_per_group(_, Config) -> - Config. + remove_all_dynamic_shovels(Config, <<"/">>), + rabbit_ct_helpers:run_teardown_steps(Config, + rabbit_ct_client_helpers:teardown_steps() ++ + rabbit_ct_broker_helpers:teardown_steps()); end_per_group(_, Config) -> - Config. + remove_all_dynamic_shovels(Config, <<"/">>), + rabbit_ct_helpers:run_teardown_steps(Config, + rabbit_ct_client_helpers:teardown_steps() ++ + rabbit_ct_broker_helpers:teardown_steps()). + +init_per_testcase(create_and_delete_a_dynamic_shovel_that_fails_to_connect = Testcase, Config) -> + case rabbit_ct_helpers:is_mixed_versions() of + true -> + {skip, "not mixed versions compatible"}; + _ -> + rabbit_ct_helpers:testcase_started(Config, Testcase) + end; init_per_testcase(Testcase, Config) -> rabbit_ct_helpers:testcase_started(Config, Testcase). -end_per_testcase(amqp10_shovels = Testcase, Config) -> - http_delete(Config, "/parameters/shovel/%2f/my-dynamic-amqp10", ?NO_CONTENT), - rabbit_ct_helpers:testcase_finished(Config, Testcase); -end_per_testcase(shovels = Testcase, Config) -> - http_delete(Config, "/vhosts/v", ?NO_CONTENT), - http_delete(Config, "/users/admin", ?NO_CONTENT), - http_delete(Config, "/users/mon", ?NO_CONTENT), - rabbit_ct_helpers:testcase_finished(Config, Testcase); end_per_testcase(Testcase, Config) -> rabbit_ct_helpers:testcase_finished(Config, Testcase). @@ -89,17 +128,22 @@ configure_shovels(Config) -> ]}). start_inets(Config) -> - ok = application:start(inets), + _ = application:start(inets), Config. %% ------------------------------------------------------------------- -%% Testcases. +%% Testcases %% ------------------------------------------------------------------- -amqp10_shovels(Config) -> +start_and_list_a_dynamic_amqp10_shovel(Config) -> Port = integer_to_binary( rabbit_ct_broker_helpers:get_node_config(Config, 0, tcp_port_amqp)), - http_put(Config, "/parameters/shovel/%2f/my-dynamic-amqp10", + + remove_all_dynamic_shovels(Config, <<"/">>), + ID = {<<"/">>, <<"dynamic-amqp10-1">>}, + await_shovel_removed(Config, ID), + + http_put(Config, "/parameters/shovel/%2f/dynamic-amqp10-1", #{value => #{'src-protocol' => <<"amqp10">>, 'src-uri' => <<"amqp://localhost:", Port/binary>>, 'src-address' => <<"test">>, @@ -109,29 +153,10 @@ amqp10_shovels(Config) -> 'dest-properties' => #{}, 'dest-application-properties' => #{}, 'dest-message-annotations' => #{}}}, ?CREATED), - % sleep to give the shovel time to emit a full report - % that includes the protocols used. - wait_until(fun () -> - case lists:sort(fun(#{name := AName}, #{name := BName}) -> - AName < BName - end, - http_get(Config, "/shovels", "guest", "guest", ?OK)) - of - [#{name := <<"my-dynamic-amqp10">>, - src_protocol := <<"amqp10">>, - dest_protocol := <<"amqp10">>, - type := <<"dynamic">>}, - #{name := <<"my-static">>, - src_protocol := <<"amqp091">>, - dest_protocol := <<"amqp091">>, - type := <<"static">>}] -> - true; - _ -> - false - end - end, 20), - ok. + await_shovel_startup(Config, ID), + + ok. -define(StaticPattern, #{name := <<"my-static">>, type := <<"static">>}). @@ -144,7 +169,7 @@ amqp10_shovels(Config) -> vhost := <<"v">>, type := <<"dynamic">>}). -shovels(Config) -> +start_static_shovels(Config) -> http_put(Config, "/users/admin", #{password => <<"admin">>, tags => <<"administrator">>}, ?CREATED), http_put(Config, "/users/mon", @@ -209,9 +234,56 @@ shovels(Config) -> http_get(Config, "/shovels/v", "mon", "mon", ?OK)), ok. -%% It's a bit arbitrary to be testing this here, but we want to be -%% able to test that mgmt extensions can be started and stopped -%% *somewhere*, and here is as good a place as any. +create_and_delete_a_dynamic_shovel_that_successfully_connects(Config) -> + Port = integer_to_binary( + rabbit_ct_broker_helpers:get_node_config(Config, 0, tcp_port_amqp)), + + remove_all_dynamic_shovels(Config, <<"/">>), + Name = <<"dynamic-amqp10-to-delete-1">>, + ID = {<<"/">>, Name}, + await_shovel_removed(Config, ID), + + http_put(Config, "/parameters/shovel/%2f/dynamic-amqp10-to-delete-1", + #{value => #{'src-protocol' => <<"amqp10">>, + 'src-uri' => <<"amqp://localhost:", Port/binary>>, + 'src-address' => <<"test">>, + 'dest-protocol' => <<"amqp10">>, + 'dest-uri' => <<"amqp://localhost:", Port/binary>>, + 'dest-address' => <<"test2">>, + 'dest-properties' => #{}, + 'dest-application-properties' => #{}, + 'dest-message-annotations' => #{}}}, ?CREATED), + + await_shovel_startup(Config, ID), + timer:sleep(3_000), + delete_shovel(Config, Name), + await_shovel_removed(Config, ID). + +create_and_delete_a_dynamic_shovel_that_fails_to_connect(Config) -> + Port = integer_to_binary( + rabbit_ct_broker_helpers:get_node_config(Config, 0, tcp_port_amqp)), + + remove_all_dynamic_shovels(Config, <<"/">>), + Name = <<"dynamic-amqp10-to-delete-2">>, + ID = {<<"/">>, Name}, + await_shovel_removed(Config, ID), + + http_put(Config, "/parameters/shovel/%2f/dynamic-amqp10-to-delete-2", + #{value => #{'src-protocol' => <<"amqp10">>, + 'src-uri' => <<"amqp://non-existing-hostname.lolz.wut:", Port/binary>>, + 'src-address' => <<"test">>, + 'dest-protocol' => <<"amqp10">>, + 'dest-uri' => <<"amqp://non-existing-hostname.lolz.wut:", Port/binary>>, + 'dest-address' => <<"test2">>, + 'dest-properties' => #{}, + 'dest-application-properties' => #{}, + 'dest-message-annotations' => #{}}}, ?CREATED), + + await_shovel_startup(Config, ID), + timer:sleep(3_000), + delete_shovel(Config, Name), + await_shovel_removed(Config, ID). + dynamic_plugin_enable_disable(Config) -> http_get(Config, "/shovels", ?OK), rabbit_ct_broker_helpers:disable_plugin(Config, 0, @@ -220,8 +292,8 @@ dynamic_plugin_enable_disable(Config) -> http_get(Config, "/overview", ?OK), rabbit_ct_broker_helpers:disable_plugin(Config, 0, "rabbitmq_management"), - http_fail(Config, "/shovels"), - http_fail(Config, "/overview"), + http_get_fails(Config, "/shovels"), + http_get_fails(Config, "/overview"), rabbit_ct_broker_helpers:enable_plugin(Config, 0, "rabbitmq_management"), http_get(Config, "/shovels", ?NOT_FOUND), @@ -232,97 +304,9 @@ dynamic_plugin_enable_disable(Config) -> http_get(Config, "/overview", ?OK), passed. -%%--------------------------------------------------------------------------- -%% TODO this is mostly copypasta from the mgmt tests - -http_get(Config, Path) -> - http_get(Config, Path, ?OK). - -http_get(Config, Path, CodeExp) -> - http_get(Config, Path, "guest", "guest", CodeExp). - -http_get(Config, Path, User, Pass, CodeExp) -> - {ok, {{_HTTP, CodeAct, _}, Headers, ResBody}} = - req(Config, get, Path, [auth_header(User, Pass)]), - assert_code(CodeExp, CodeAct, "GET", Path, ResBody), - decode(CodeExp, Headers, ResBody). - -http_fail(Config, Path) -> - {error, {failed_connect, _}} = req(Config, get, Path, []). - -http_put(Config, Path, List, CodeExp) -> - http_put_raw(Config, Path, format_for_upload(List), CodeExp). - -http_put(Config, Path, List, User, Pass, CodeExp) -> - http_put_raw(Config, Path, format_for_upload(List), User, Pass, CodeExp). - -http_post(Config, Path, List, CodeExp) -> - http_post_raw(Config, Path, format_for_upload(List), CodeExp). - -http_post(Config, Path, List, User, Pass, CodeExp) -> - http_post_raw(Config, Path, format_for_upload(List), User, Pass, CodeExp). - -format_for_upload(none) -> - <<"">>; -format_for_upload(Map) -> - iolist_to_binary(rabbit_json:encode(convert_keys(Map))). - -convert_keys(Map) -> - maps:fold(fun - (K, V, Acc) when is_map(V) -> - Acc#{atom_to_binary(K, latin1) => convert_keys(V)}; - (K, V, Acc) -> - Acc#{atom_to_binary(K, latin1) => V} - end, #{}, Map). - -http_put_raw(Config, Path, Body, CodeExp) -> - http_upload_raw(Config, put, Path, Body, "guest", "guest", CodeExp). - -http_put_raw(Config, Path, Body, User, Pass, CodeExp) -> - http_upload_raw(Config, put, Path, Body, User, Pass, CodeExp). - -http_post_raw(Config, Path, Body, CodeExp) -> - http_upload_raw(Config, post, Path, Body, "guest", "guest", CodeExp). - -http_post_raw(Config, Path, Body, User, Pass, CodeExp) -> - http_upload_raw(Config, post, Path, Body, User, Pass, CodeExp). - -http_upload_raw(Config, Type, Path, Body, User, Pass, CodeExp) -> - {ok, {{_HTTP, CodeAct, _}, Headers, ResBody}} = - req(Config, Type, Path, [auth_header(User, Pass)], Body), - assert_code(CodeExp, CodeAct, Type, Path, ResBody), - decode(CodeExp, Headers, ResBody). - -http_delete(Config, Path, CodeExp) -> - http_delete(Config, Path, "guest", "guest", CodeExp). - -http_delete(Config, Path, User, Pass, CodeExp) -> - {ok, {{_HTTP, CodeAct, _}, Headers, ResBody}} = - req(Config, delete, Path, [auth_header(User, Pass)]), - assert_code(CodeExp, CodeAct, "DELETE", Path, ResBody), - decode(CodeExp, Headers, ResBody). - -assert_code(CodeExp, CodeAct, _Type, _Path, _Body) -> - ?assertEqual(CodeExp, CodeAct). - -req_uri(Config, Path) -> - rabbit_misc:format("~ts/api~ts", [ - rabbit_ct_broker_helpers:node_uri(Config, 0, management), - Path - ]). - -req(Config, Type, Path, Headers) -> - httpc:request(Type, - {req_uri(Config, Path), Headers}, - ?HTTPC_OPTS, []). - -req(Config, Type, Path, Headers, Body) -> - httpc:request(Type, - {req_uri(Config, Path), Headers, "application/json", Body}, - ?HTTPC_OPTS, []). - -decode(?OK, _Headers, ResBody) -> cleanup(rabbit_json:decode(rabbit_data_coercion:to_binary(ResBody))); -decode(_, Headers, _ResBody) -> Headers. +%% +%% Implementation +%% cleanup(L) when is_list(L) -> [cleanup(I) || I <- L]; @@ -345,13 +329,37 @@ assert_item(ExpI, ActI) -> ExpI = maps:with(maps:keys(ExpI), ActI), ok. -wait_until(_Fun, 0) -> - ?assert(wait_failed); -wait_until(Fun, N) -> - case Fun() of - true -> - ok; - false -> - timer:sleep(500), - wait_until(Fun, N - 1) - end. +delete_shovel(Config, Name) -> + Path = io_lib:format("/shovels/vhost/%2F/~s", [Name]), + http_delete(Config, Path, ?NO_CONTENT). + +remove_all_dynamic_shovels(Config, VHost) -> + rabbit_ct_broker_helpers:rpc(Config, 0, + rabbit_runtime_parameters, clear_vhost, [VHost, <<"CT tests">>]). + +await_shovel_startup(Config, Name) -> + await_shovel_startup(Config, Name, 10_000). + +await_shovel_startup(Config, Name, Timeout) -> + await_condition( + fun() -> + does_shovel_exist(Config, Name) + end, Timeout). + +await_shovel_removed(Config, Name) -> + await_shovel_removed(Config, Name, 10_000). + +await_shovel_removed(Config, Name, Timeout) -> + await_condition( + fun() -> + not does_shovel_exist(Config, Name) + end, Timeout). + +lookup_shovel_status(Config, Name) -> + rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_shovel_status, lookup, [Name]). + +does_shovel_exist(Config, Name) -> + case lookup_shovel_status(Config, Name) of + not_found -> false; + _Found -> true + end. \ No newline at end of file diff --git a/deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_util_SUITE.erl b/deps/rabbitmq_shovel_management/test/unit_SUITE.erl similarity index 98% rename from deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_util_SUITE.erl rename to deps/rabbitmq_shovel_management/test/unit_SUITE.erl index f4426c875792..b7ea700ed2ad 100644 --- a/deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_util_SUITE.erl +++ b/deps/rabbitmq_shovel_management/test/unit_SUITE.erl @@ -1,5 +1,5 @@ %%% @doc Unit tests of rabbit_shovel_mgmt_util --module(rabbit_shovel_mgmt_util_SUITE). +-module(unit_SUITE). -compile([export_all, nowarn_export_all]). From 11e23071cdc65e4b8017fd795c116242d00c48ee Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Fri, 24 May 2024 21:17:37 -0400 Subject: [PATCH 2/2] Resolve conflicts #11321 #11324 --- .../test/http_SUITE.erl | 12 -- .../test/rabbit_shovel_mgmt_SUITE.erl | 103 ------------------ 2 files changed, 115 deletions(-) delete mode 100644 deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_SUITE.erl diff --git a/deps/rabbitmq_shovel_management/test/http_SUITE.erl b/deps/rabbitmq_shovel_management/test/http_SUITE.erl index 0a053cc66464..07d294086a5f 100644 --- a/deps/rabbitmq_shovel_management/test/http_SUITE.erl +++ b/deps/rabbitmq_shovel_management/test/http_SUITE.erl @@ -18,24 +18,13 @@ all() -> [ -<<<<<<< HEAD - {group, non_parallel_tests} -======= {group, dynamic_shovels}, {group, static_shovels}, {group, plugin_management} ->>>>>>> cae964dba1 (ctl delete_shovel: use a more effective way) ]. groups() -> [ -<<<<<<< HEAD - {non_parallel_tests, [], [ - amqp10_shovels, - shovels, - dynamic_plugin_enable_disable - ]} -======= {dynamic_shovels, [], [ start_and_list_a_dynamic_amqp10_shovel, create_and_delete_a_dynamic_shovel_that_successfully_connects, @@ -49,7 +38,6 @@ groups() -> {plugin_management, [], [ dynamic_plugin_enable_disable ]} ->>>>>>> cae964dba1 (ctl delete_shovel: use a more effective way) ]. %% ------------------------------------------------------------------- diff --git a/deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_SUITE.erl b/deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_SUITE.erl deleted file mode 100644 index 1953aa35851c..000000000000 --- a/deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_SUITE.erl +++ /dev/null @@ -1,103 +0,0 @@ --module(rabbit_shovel_mgmt_SUITE). - --include_lib("common_test/include/ct.hrl"). --include_lib("eunit/include/eunit.hrl"). --include_lib("rabbit_common/include/rabbit.hrl"). --include_lib("rabbitmq_management_agent/include/rabbit_mgmt_records.hrl"). - --compile(export_all). - --define(MOCK_SHOVELS, - [[ - {node,node()}, - {name,<<"shovel1">>}, - {vhost,<<"/">>}, - {type,dynamic}, - {state,running}, - {src_uri,<<"amqp://">>}, - {src_protocol,<<"amqp091">>}, - {dest_protocol,<<"amqp091">>}, - {dest_uri,<<"amqp://">>}, - {src_queue,<<"q1">>}, - {dest_queue,<<"q2">>} - ], - [ - {node,'node2'}, - {name,<<"shovel2">>}, - {vhost,<<"otherVhost">>}, - {type,dynamic}, - {state,running}, - {src_uri,<<"amqp://">>}, - {src_protocol,<<"amqp091">>}, - {dest_protocol,<<"amqp091">>}, - {dest_uri,<<"amqp://">>}, - {src_queue,<<"q1">>}, - {dest_queue,<<"q2">>} - ]]). - -all() -> - [ - get_shovel_node_shovel_different_name, - get_shovel_node_shovel_different_vhost_name, - get_shovel_node_shovel_found, - delete_resource_badrpc - ]. - -init_per_testcase(delete_resource_badrpc, _Config) -> - meck:expect(rabbit_shovel_mgmt_util, status, fun(_,_) -> ?MOCK_SHOVELS end), - meck:expect(rabbit_shovel_status, lookup, - fun({_, Name}) -> - case [S || S <- ?MOCK_SHOVELS, proplists:get_value(name, S) =:= Name] of - [Obj] -> Obj; - [] -> not_found - end - end), - _Config; -init_per_testcase(_, _Config) -> - meck:new(rabbit_shovel_mgmt_util), - meck:expect(rabbit_shovel_mgmt_util, status, fun(_,_) -> ?MOCK_SHOVELS end), - _Config. - -end_per_testcase(delete_resource_badrpc, _Config) -> - meck:unload(rabbit_shovel_mgmt_util), - meck:unload(rabbit_shovel_status), - _Config; -end_per_testcase(_, _Config) -> - meck:unload(rabbit_shovel_mgmt_util), - _Config. - -get_shovel_node_shovel_different_name(_Config) -> - VHost = <<"otherVhost">>, - Name= <<"shovelThatDoesntExist">>, - User = #user{username="admin",tags = [administrator]}, - Node = rabbit_shovel_mgmt:get_shovel_node(VHost, Name, {}, #context{user = User}), - ?assertEqual(undefined, Node). - -get_shovel_node_shovel_different_vhost_name(_Config) -> - VHost = <<"VHostThatDoesntExist">>, - Name= <<"shovel1">>, - User = #user{username="admin",tags = [administrator]}, - Node = rabbit_shovel_mgmt:get_shovel_node(VHost, Name, {}, #context{user = User}), - ?assertEqual(undefined, Node). - -get_shovel_node_shovel_found(_Config) -> - VHost = <<"otherVhost">>, - Name= <<"shovel2">>, - User = #user{username="admin",tags = [administrator]}, - Node = rabbit_shovel_mgmt:get_shovel_node(VHost, Name, {}, #context{user = User}), - ?assertEqual('node2', Node). - -delete_resource_badrpc(_Config) -> - VHost = <<"/">>, - Name= <<"shovel1">>, - User = #user{username="admin",tags = [administrator]}, - Context = #context{user = User}, - ReqData = #{path => <<"/shovels/vhost/././restart">>, - bindings => #{vhost => VHost, name => Name}}, - {Reply, ReqData, Context} = rabbit_shovel_mgmt:delete_resource(ReqData, Context), - ?assertEqual(false, Reply), - - ReqData2 = #{path => <<"/shovels/vhost/./.">>, - bindings => #{vhost => VHost, name => Name}}, - {Reply, ReqData2, Context} = rabbit_shovel_mgmt:delete_resource(ReqData2, Context), - ?assertEqual(false, Reply).