Merge pull request #185 from esl/cosmetics

Cosmetics changes to improve readability, types, and upgrades

Author: DenysGonchar

.github/workflows/ci.yml

@@ -4,17 +4,18 @@ on:
   push:
     branches: [ master ]
   pull_request:
-    branches: [ master ]
+  workflow_dispatch:
+
 
 jobs:
   test:
     name: ${{ matrix.test-type }} test on OTP ${{matrix.otp_vsn}}
     strategy:
       matrix:
-        otp_vsn: ['26.2', '25.3', '24.3']
-        rebar_vsn: ['3.22.0']
+        otp_vsn: ['27', '26', '25']
+        rebar_vsn: ['3.23.0']
         test-type: ['regular', 'integration']
-    runs-on: 'ubuntu-22.04'
+    runs-on: 'ubuntu-24.04'
     steps:
       - uses: actions/checkout@v4
       - uses: erlef/setup-beam@v1

guides/configuration.md

@@ -9,9 +9,6 @@ Amoc supports the following generic configuration parameters:
     * default value - empty list (`[]`)
     * example: `AMOC_NODES="['amoc@amoc-1', 'amoc@amoc-2']"`
 
-* `api_port` - a port for the amoc REST interfaces:
-    * default value - 4000
-    * example: `AMOC_API_PORT="4000"`
 
 * `interarrival` - a delay (in ms, for each node in the cluster independently) between creating the processes
   for two consecutive users:

integration_test/extra_code_paths/path1/dummy_helper.erl

@@ -1,7 +1,12 @@
 -module(dummy_helper).
 
--required_variable(#{name=>dummy_var, description=>"dummy_var",
-                     default_value=>default_value}).
+-include_lib("stdlib/include/assert.hrl").
+
+-required_variable(#{name => dummy_var,
+                     description => "dummy_var",
+                     default_value => default_value}).
+
+-define(comment(U), io_lib:format("Condition failed with last users distribution ~n~p", [U])).
 
 %% amoc_dist testing function
 -export([test_amoc_dist/0]).
@@ -11,65 +16,73 @@ test_amoc_dist() ->
         Master = amoc_cluster:master_node(),
         Slaves = amoc_cluster:slave_nodes(),
         %% check the status of the nodes
-        disabled = rpc:call(Master, amoc_controller, get_status, []),
-        [{running, #{scenario := dummy_scenario}} = rpc:call(Node, amoc_controller, get_status, [])
-         || Node <- Slaves],
-        %% check user ids
-        {N1, Nodes1, Ids1, Max1} = get_users_info(Slaves),
-        true = N1 > 0,
-        N1 = Max1,
-        Ids1 = lists:seq(1, N1),
+        ?assertEqual(disabled, get_status(Master)),
+        [ ?assertMatch({running, #{scenario := dummy_scenario}}, get_status(Node)) || Node <- Slaves],
+        %% check user ids, users have all been started at the first two nodes
+        {N1, Max1, Nodes1, Ids1, Users1} = get_users_info(Slaves),
+        ?assert(N1 > 0),
+        ?assertEqual(N1, Max1, ?comment(Users1)),
+        ?assertEqual(Ids1, lists:seq(1, N1), ?comment(Users1)),
         [AddedNode] = Slaves -- Nodes1,
         %% add 20 users
-        {ok, _} = rpc:call(Master, amoc_dist, add, [20]),
-        timer:sleep(3000),
-        {N2, Nodes2, Ids2, Max2} = get_users_info(Slaves),
-        N2 = Max2,
-        Ids2 = lists:seq(1, N2),
-        [AddedNode] = Nodes2 -- Nodes1,
-        N2 = N1 + 20,
+        add_and_wait(Master, 20),
+        {N2, Max2, Nodes2, Ids2, Users2} = get_users_info(Slaves),
+        ?assertEqual(N2, Max2, ?comment(Users2)),
+        ?assertEqual(Ids2, lists:seq(1, N2), ?comment(Users2)),
+        ?assertEqual([AddedNode], Nodes2 -- Nodes1, ?comment(Users2)),
+        ?assertEqual(N2, N1 + 20, ?comment(Users2)),
         %% remove 10 users
-        {ok, _} = rpc:call(Master, amoc_dist, remove, [10, true]),
-        timer:sleep(3000),
-        {N3, Nodes3, _Ids3, Max3} = get_users_info(Slaves),
-        Nodes2 = Nodes3,
-        Max3 = Max2,
-        N2 = N3 + 10,
+        remove_and_wait(Master, 10),
+        {N3, Max3, Nodes3, _Ids3, Users3} = get_users_info(Slaves),
+        ?assertEqual(N2 - 10, N3, ?comment(Users3)),
+        ?assertEqual(Max2, Max3, ?comment(Users3)),
+        ?assertEqual(Nodes2, Nodes3, ?comment(Users3)),
         %% try to remove N3 users
-        {ok, Ret} = rpc:call(Master, amoc_dist, remove, [N3, true]),
+        Ret = remove_and_wait(Master, N3),
         RemovedN = lists:sum([N || {_, N} <- Ret]),
-        timer:sleep(3000),
-        {N4, Nodes4, Ids4, _Max4} = get_users_info(Slaves),
-        Nodes1 = Nodes4,
-        N3 = N4 + RemovedN,
-        true = RemovedN < N3,
+        {N4, _Max4, Nodes4, Ids4, Users4} = get_users_info(Slaves),
+        ?assertEqual(N3 - RemovedN, N4, ?comment(Users4)),
+        ?assertEqual(Nodes1, Nodes4, ?comment(Users4)),
+        ?assert(RemovedN < N3),
         %% add 20 users
-        {ok, _} = rpc:call(Master, amoc_dist, add, [20]),
-        timer:sleep(3000),
-        {N5, Nodes5, Ids5, Max5} = get_users_info(Slaves),
-        Nodes2 = Nodes5,
-        Max5 = Max2 + 20,
-        N5 = N4 + 20,
-        true = Ids5 -- Ids4 =:= lists:seq(Max2 + 1, Max5),
+        add_and_wait(Master, 20),
+        {N5, Max5, Nodes5, Ids5, Users5} = get_users_info(Slaves),
+        ?assertEqual(Nodes2, Nodes5, ?comment(Users5)),
+        ?assertEqual(Max5, Max2 + 20, ?comment(Users5)),
+        ?assertEqual(N5, N4 + 20, ?comment(Users5)),
+        ?assertEqual(Ids5 -- Ids4, lists:seq(Max2 + 1, Max5), ?comment(Users5)),
         %% terminate scenario
-        {ok,_} = rpc:call(Master, amoc_dist, stop, []),
-        timer:sleep(3000),
-        [{finished, dummy_scenario} = rpc:call(Node, amoc_controller, get_status, [])
-         || Node <- Slaves],
+        stop(Master),
+        [ ?assertEqual({finished, dummy_scenario}, get_status(Node)) || Node <- Slaves],
         %% return expected value
         amoc_dist_works_as_expected
     catch
         C:E:S ->
-            {error, {C, E, S}}
+            {C, E, S}
     end.
 
 get_users_info(SlaveNodes) ->
-    Users = [{Node, Id} ||
-        Node <- SlaveNodes,
-        {_Pid, Id} <- rpc:call(Node, amoc_users_sup, get_all_children, [])],
-    Ids = lists:usort([Id || {_, Id} <- Users]),
-    Nodes = lists:usort([Node || {Node, _} <- Users]),
+    Distrib = [ {Node, erpc:call(Node, amoc_users_sup, get_all_children, [])} || Node <- SlaveNodes ],
+    Ids = lists:usort([Id || {_Node, Users} <- Distrib, {_, Id} <- Users]),
+    Nodes = lists:usort([Node || {Node, Users} <- Distrib, [] =/= Users]),
     N = length(Ids),
-    N = length(Users),
     MaxId = lists:max(Ids),
-    {N, Nodes, Ids, MaxId}.
+    {N, MaxId, Nodes, Ids, Distrib}.
+
+add_and_wait(Master, Num) ->
+    {ok, Ret} = erpc:call(Master, amoc_dist, add, [Num]),
+    timer:sleep(3000),
+    Ret.
+
+remove_and_wait(Master, Num) ->
+    {ok, Ret} = erpc:call(Master, amoc_dist, remove, [Num, true]),
+    timer:sleep(3000),
+    Ret.
+
+stop(Master) ->
+    {ok, Ret} = erpc:call(Master, amoc_dist, stop, []),
+    timer:sleep(3000),
+    Ret.
+
+get_status(Node) ->
+    erpc:call(Node, amoc_controller, get_status, []).

src/amoc_controller.erl

@@ -79,7 +79,7 @@
 %% ------------------------------------------------------------------
 
 %% @private
--spec start_link() -> {ok, pid()}.
+-spec start_link() -> gen_server:start_ret().
 start_link() ->
     gen_server:start_link({local, ?SERVER}, ?MODULE, [], []).
 

src/amoc_scenario.erl

@@ -73,7 +73,7 @@ terminate(Scenario, State) ->
 %% if scenario module exports both functions, `Scenario:start/2' is used.
 %%
 %% Runs on the user process and spans a `[amoc, scenario, user, _]' telemetry event.
--spec start(amoc:scenario(), user_id(), state()) -> any().
+-spec start(amoc:scenario(), user_id(), state()) -> term().
 start(Scenario, Id, State) ->
     Metadata = #{scenario => Scenario, state => State, user_id => Id},
     Span = case {erlang:function_exported(Scenario, start, 2),

src/amoc_sup.erl

@@ -11,14 +11,14 @@
 -export([init/1]).
 
 %% Helper macro for declaring children of supervisor
--define(WORKER(I, Type), {I, {I, start_link, []}, permanent, 5000, Type, [I]}).
--define(SUP(I, Type), {I, {I, start_link, []}, permanent, infinity, Type, [I]}).
+-define(WORKER(I), {I, {I, start_link, []}, permanent, 5000, worker, [I]}).
+-define(SUP(I), {I, {I, start_link, []}, permanent, infinity, supervisor, [I]}).
 
 %% ===================================================================
 %% API functions
 %% ===================================================================
 
--spec start_link() -> {ok, Pid :: pid()}.
+-spec start_link() -> supervisor:startlink_ret().
 start_link() ->
     supervisor:start_link({local, ?MODULE}, ?MODULE, []).
 
@@ -30,10 +30,10 @@ start_link() ->
 init([]) ->
     {ok, {#{strategy => one_for_all, intensity => 0},
           [
-              ?SUP(amoc_users_sup, supervisor),
-              ?SUP(amoc_throttle_sup, supervisor),
-              ?SUP(amoc_coordinator_sup, supervisor),
-              ?WORKER(amoc_controller, worker),
-              ?WORKER(amoc_cluster, worker),
-              ?WORKER(amoc_code_server, worker)
+              ?SUP(amoc_users_sup),
+              ?SUP(amoc_throttle_sup),
+              ?SUP(amoc_coordinator_sup),
+              ?WORKER(amoc_controller),
+              ?WORKER(amoc_cluster),
+              ?WORKER(amoc_code_server)
           ]}}.

src/dist/amoc_dist.erl

@@ -94,7 +94,7 @@ get_state() ->
     case {amoc_cluster:master_node(), get_param(state)} of
         {undefined, undefined} -> idle;
         {_, {ok, State}} -> State;
-        {Node, undefined} -> rpc:call(Node, ?MODULE, ?FUNCTION_NAME, [])
+        {Node, undefined} -> erpc:call(Node, ?MODULE, ?FUNCTION_NAME, [])
     end.
 
 %% ------------------------------------------------------------------
@@ -172,7 +172,7 @@ setup_slave_node(Node) ->
         {ok, _} ->
             {ok, Scenario} = get_param(scenario),
             {ok, Settings} = get_param(settings),
-            rpc:call(Node, amoc_controller, start_scenario, [Scenario, Settings]);
+            erpc:call(Node, amoc_controller, start_scenario, [Scenario, Settings]);
         Error -> Error
     end.
 
@@ -196,7 +196,7 @@ add_users(Result, LastId, Count, [Node | T] = Nodes) ->
         0 ->
             add_users([{Node, {ok, node_skipped}} | Result], LastId, Count, T);
         N ->
-            Ret = rpc:call(Node, amoc_controller, add_users, [LastId + 1, LastId + N]),
+            Ret = erpc:call(Node, amoc_controller, add_users, [LastId + 1, LastId + N]),
             add_users([{Node, Ret} | Result], LastId + N, Count - N, T)
     end.
 
@@ -213,7 +213,7 @@ remove_users(Result, Count, ForceRemove, [Node | T] = Nodes) ->
         0 ->
             remove_users([{Node, {ok, node_skipped}} | Result], Count, ForceRemove, T);
         N ->
-            Ret = rpc:call(Node, amoc_controller, remove_users, [N, ForceRemove]),
+            Ret = erpc:call(Node, amoc_controller, remove_users, [N, ForceRemove]),
             remove_users([{Node, Ret} | Result], Count - N, ForceRemove, T)
     end.
 
@@ -225,7 +225,7 @@ update_settings_on_nodes(Settings, Nodes) ->
 -spec update_settings_on_node(amoc_config:settings(), node()) ->
           ok | {badrpc, any()} | {error, any()}.
 update_settings_on_node(Settings, Node) ->
-    rpc:call(Node, amoc_controller, update_settings, [Settings]).
+    erpc:call(Node, amoc_controller, update_settings, [Settings]).
 
 -spec stop_cluster() -> {ok, any()} | {error, any()}.
 stop_cluster() ->
@@ -234,7 +234,7 @@ stop_cluster() ->
         {_, []} -> {error, no_slave_nodes};
         {MasterNode, Slaves} ->
             set_state(stopped),
-            Result = [{Node, rpc:call(Node, amoc_controller, stop_scenario, [])} ||
+            Result = [{Node, erpc:call(Node, amoc_controller, stop_scenario, [])} ||
                          Node <- Slaves],
             maybe_error(Result);
         {_, _} -> {error, not_a_master}

src/throttle/amoc_throttle_process.erl

@@ -24,7 +24,7 @@
          handle_info/2,
          handle_cast/2,
          handle_continue/2,
-         format_status/2]).
+         format_status/1]).
 
 -define(PG_SCOPE, amoc_throttle).
 -define(DEFAULT_MSG_TIMEOUT, 60000).%% one minute
@@ -143,12 +143,13 @@ handle_continue(maybe_run_fn, State) ->
     NewState = maybe_run_fn(State),
     {noreply, NewState, timeout(NewState)}.
 
--spec format_status(term(), term()) -> term().
-format_status(_Opt, [_PDict, State]) ->
+-spec format_status(gen_server:format_status()) -> gen_server:format_status().
+format_status(#{state := #state{} = State} = FormatStatus) ->
     ScheduleLen = length(State#state.schedule),
     ScheduleRevLen = length(State#state.schedule_reversed),
     State1 = setelement(#state.schedule, State, ScheduleLen),
-    setelement(#state.schedule_reversed, State1, ScheduleRevLen).
+    State2 = setelement(#state.schedule_reversed, State1, ScheduleRevLen),
+    FormatStatus#{state := State2}.
 
 %%------------------------------------------------------------------------------
 %% internal functions

src/users/amoc_user.erl

@@ -14,16 +14,15 @@
 start_link(Scenario, Id, State) ->
     proc_lib:start_link(?MODULE, init, [self(), Scenario, Id, State]).
 
--spec stop() -> no_return().
+-spec stop() -> ok.
 stop() ->
     stop(self(), false).
 
 -spec stop(pid(), boolean()) -> ok.
 stop(Pid, Force) when is_pid(Pid) ->
     amoc_users_sup:stop_child(Pid, Force).
 
--spec init(pid(), amoc:scenario(), amoc_scenario:user_id(), state()) ->
-    no_return().
+-spec init(pid(), amoc:scenario(), amoc_scenario:user_id(), state()) -> term().
 init(Parent, Scenario, Id, State) ->
     proc_lib:init_ack(Parent, {ok, self()}),
     process_flag(trap_exit, true),

src/users/amoc_users_sup.erl

@@ -139,14 +139,14 @@ terminate_all_children() ->
 -spec get_sup_for_user_id(amoc_scenario:user_id()) -> pid().
 get_sup_for_user_id(Id) ->
     #storage{sups = Supervisors, sups_count = SupCount} = persistent_term:get(?MODULE),
-    Index = Id rem SupCount + 1,
+    Index = erlang:phash2(Id, SupCount) + 1,
     element(Index, Supervisors).
 
 %% assign which users each worker will be requested to add
 -spec assign_users_to_sups(pos_integer(), tuple(), [amoc_scenario:user_id()], Acc) ->
     Acc when Acc :: #{pid() := [amoc_scenario:user_id()]}.
 assign_users_to_sups(SupCount, Supervisors, [Id | Ids], Acc) ->
-    Index = Id rem SupCount + 1,
+    Index = erlang:phash2(Id, SupCount) + 1,
     ChosenSup = element(Index, Supervisors),
     Vs = maps:get(ChosenSup, Acc),
     NewAcc = Acc#{ChosenSup := [Id | Vs]},