Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modify and improve config change callback system #74

Merged
merged 6 commits into from
Aug 12, 2016
Merged

Modify and improve config change callback system #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e58775
Update README to reflect new cfg callback system
nickelization Aug 9, 2016
4740e5d
Modify "set" to use new config callback style
nickelization Aug 9, 2016
eb5dec5
Improve error handling for badrpc errors
nickelization Aug 9, 2016
dadc5e1
Improve output for "set" command
nickelization Aug 11, 2016
5970a9f
Simplify the run_callback function
nickelization Aug 11, 2016
14e01b0
Add full suite of unit tests for the "set" command
nickelization Aug 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,19 +170,27 @@ exceed the new limit.
Configuration specific behaviour can be managed by registering a callback to
fire when a given configuration variable is set on the cli. The callback runs
*after* the corresponding environment variables are set. The callback function
is a 3-arity function that gets called with the original key (as a list of
strings()), the untranslated value to set (as a string()) and the flags passed
on the command-line. The flags can be either '--all' to run on all nodes, or
--node N to run on node N. If no flags are given the command should be executed
on the local node (where the cli command was run) only.
is a 2-arity function that gets called with the original key (as a list of
strings()), and the untranslated value to set (as a string()).

On the command-line, the flags can be either '--all' to run on all nodes, or
--node N to run on node N instead of the local node. If no flags are given,
the config change will take place on the local node (where the cli command was
run) only. These flags are not visible to the callback function; rather, the
callback function will be called on whichever nodes the config change is being
made.

Unlike command callbacks, config callbacks need only return a short iolist
describing any immediate results of the config change that may have taken
place. This allows results from --all to be compiled into a table, and lets
results from other invocations be displayed via simple status messages.

```erlang
-spec set_transfer_limit(Key :: [string()], Val :: string(),
Flags :: [{atom(), proplist()}]).
-spec set_transfer_limit(Key :: [string()], Val :: string()) -> Result :: string().
...

Key = ["transfer_limit"],
Callback = fun set_transfer_limit/3,
Callback = fun set_transfer_limit/2,
clique:register_config(Key, Callback).
```

Expand Down
274 changes: 178 additions & 96 deletions src/clique_config.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@
load_schema/1]).

%% Callbacks for rpc calls
-export([get_local_env_status/2,
get_local_env_vals/2,
set_local_app_config/1]).
-export([do_set/1,
get_local_env_status/2,
get_local_env_vals/2]).

-define(config_table, clique_config).
-define(schema_table, clique_schema).
Expand All @@ -47,6 +47,7 @@
-type flagspecs() :: [spec()].
-type flags() :: proplist().
-type args() :: clique_parser:args().
-type conf() :: [{[string()], string()}].

-type envkey() :: {string(), {atom(), atom()}}.
-type cuttlefish_flag_spec() :: {flag, atom(), atom()}.
Expand Down Expand Up @@ -120,8 +121,44 @@ describe(Args, _Flags) ->
end.

-spec set(proplist(), proplist()) -> status() | err().
set(Args, Flags) ->
M1 = get_config({Args, Flags}),
set(Args, [{all, _}]) ->
%% Done as an io:format instead of a status, so that the user is not totally
%% left in the dark if the multicall ends up taking a while to finish:
io:format("Setting config across the cluster~n", []),
Nodes = clique_nodes:nodes(),
{Results0, Down0} = rpc:multicall(Nodes, ?MODULE, do_set, [Args]),

Results = [[{"Node", Node}, {"Node Down/Unreachable", false}, {"Result", Status}] ||
{Node, Status} <- Results0],
Down = [[{"Node", Node}, {"Node Down/Unreachable", true}, {"Result", "N/A"}] ||
Node <- Down0],

NodeStatuses = lists:sort(Down ++ Results),
[clique_status:table(NodeStatuses)];
set(Args, [{node, NodeStr}]) ->
M1 = clique_typecast:to_node(NodeStr),
M2 = rpc_set(M1, Args),
return_set_status(M2, NodeStr);
set(Args, []) ->
M1 = do_set(Args),
return_set_status(M1, node());
set(_Args, _Flags) ->
app_config_flags_error().

rpc_set({error, _} = E, _Args) ->
E;
rpc_set(Node, Args) ->
clique_nodes:safe_rpc(Node, ?MODULE, do_set, [Args]).

return_set_status({error, _} = E, _Node) ->
E;
return_set_status({badrpc, Reason}, Node) ->
clique_error:badrpc_to_error(Node, Reason);
return_set_status({_, Result}, _Node) ->
[clique_status:text(Result)].

do_set(Args) ->
M1 = get_config(Args),
M2 = set_config(M1),
run_callback(M2).

Expand Down Expand Up @@ -151,8 +188,7 @@ check_keys_in_whitelist(Keys) ->
_ -> {error, {config_not_settable, Invalid}}
end.

-spec get_env_status([envkey()], cuttlefish_flag_list(), flags()) -> status() |
err().
-spec get_env_status([envkey()], cuttlefish_flag_list(), flags()) -> status() | err().
get_env_status(EnvKeys, CuttlefishFlags, []) ->
get_local_env_status(EnvKeys, CuttlefishFlags);
get_env_status(EnvKeys, CuttlefishFlags, Flags) when length(Flags) =:= 1 ->
Expand Down Expand Up @@ -192,15 +228,12 @@ get_local_env_vals(EnvKeys, CuttlefishFlags) ->
end || {{KeyStr, {App, Key}}, CFlagSpec} <- lists:zip(EnvKeys, CuttlefishFlags)],
[{"node", node()} | Vals].

-spec get_remote_env_status([envkey()], cuttlefish_flag_list(), node()) ->
status().
-spec get_remote_env_status([envkey()], cuttlefish_flag_list(), node()) -> status() | err().
get_remote_env_status(EnvKeys, CuttlefishFlags, Node) ->
case clique_nodes:safe_rpc(Node, ?MODULE, get_local_env_status,
[EnvKeys, CuttlefishFlags]) of
{badrpc, rpc_process_down} ->
{error, {rpc_process_down, Node}};
{badrpc, nodedown} ->
{error, {nodedown, Node}};
{badrpc, Reason} ->
clique_error:badrpc_to_error(Node, Reason);
Status ->
Status
end.
Expand All @@ -225,109 +258,57 @@ get_remote_env_status(EnvKeys, CuttlefishFlags) ->


-spec run_callback(err()) -> err();
([{[string()], string(), status()}]) -> status().
(conf()) -> {node, iolist()}.
run_callback({error, _}=E) ->
E;
run_callback({Args, Flags, Status}) ->
KVFuns = lists:foldl(fun({K, V}, Acc) ->
case ets:lookup(?config_table, K) of
[{K, F}] ->
[{K, V, F} | Acc];
[] ->
Acc
end
end, [], Args),
_ = [F(K, V, Flags) || {K, V, F} <- KVFuns],
Status.

-spec get_config(err()) -> err();
({args(), flags()}) ->
{proplist(), proplist(), flags()} | err().
get_config({error, _}=E) ->
E;
get_config({[], _Flags}) ->
run_callback(Args) ->
OutStrings = [run_callback(K, V, F) || {K, V} <- Args, {_, F} <- ets:lookup(?config_table, K)],
Output = string:join(OutStrings, "\n"), %% TODO return multiple strings tagged with keys
%% Tag the return value with our current node so we know
%% where this result came from when we use multicall:
{node(), Output}.

run_callback(K, V, F) ->
KeyString = cuttlefish_variable:format(K),
UpdateMsg = io_lib:format("~s set to ~p", [KeyString, V]),
case F(K, V) of
"" ->
UpdateMsg;
Output ->
[UpdateMsg, $\n, Output]
end.

-spec get_config(args()) -> err() | {args(), proplist(), conf()}.
get_config([]) ->
{error, set_no_args};
get_config({Args, Flags}) ->
get_config(Args) ->
[{schema, Schema}] = ets:lookup(?schema_table, schema),
Conf = [{cuttlefish_variable:tokenize(K), V} || {K, V} <- Args],
case cuttlefish_generator:minimal_map(Schema, Conf) of
{error, _, Msg} ->
{error, {invalid_config, Msg}};
AppConfig ->
{AppConfig, Conf, Flags}
{Args, AppConfig, Conf}
end.

-spec set_config(err()) -> err();
({proplist(), proplist(), flags()}) -> {proplist(), flags(), status()} | err().
({args(), proplist(), conf()}) -> err() | conf().
set_config({error, _}=E) ->
E;
set_config({AppConfig, Args, Flags}) ->
Keys = [cuttlefish_variable:format(K) || {K, _} <- Args],
set_config({Args, AppConfig, Conf}) ->
Keys = [K || {K, _} <- Args],
case check_keys_in_whitelist(Keys) of
ok ->
case set_app_config(AppConfig, Flags) of
{error, _} = E ->
E;
Status ->
{Args, Flags, Status}
end;
_ = set_app_config(AppConfig),
Conf;
{error, _}=E ->
E
end.

-spec set_app_config(proplist(), flags()) -> status() | err().
set_app_config(AppConfig, []) ->
{ok, Status} = set_local_app_config(AppConfig),
Status;
set_app_config(AppConfig, Flags) when length(Flags) =:= 1 ->
[{Key, Val}] = Flags,
case Key of
node -> set_remote_app_config(AppConfig, Val);
all -> set_remote_app_config(AppConfig)
end;
set_app_config(_AppConfig, _Flags) ->
app_config_flags_error().

-spec set_local_app_config(proplist()) -> {ok, status()}.
set_local_app_config(AppConfig) ->
_ = [application:set_env(App, Key, Val) || {App, Settings} <- AppConfig,
{Key, Val} <- Settings],
%% We return {ok, Status} instead of just Status so that we can more easily
%% differentiate a successful return from an RPC error when we call this
%% remotely.
{ok, []}.

-spec set_remote_app_config(proplist(), node()) -> status() | err().
set_remote_app_config(AppConfig, Node) ->
Fun = set_local_app_config,
case clique_nodes:safe_rpc(Node, ?MODULE, Fun, [AppConfig]) of
{badrpc, rpc_process_down} ->
{error, {rpc_process_down, Node}};
{badrpc, nodedown} ->
{error, {nodedown, Node}};
{ok, Status} ->
Status
end.

-spec set_remote_app_config(proplist()) -> status().
set_remote_app_config(AppConfig) ->
%% TODO Convert this io:format to a status? Maybe better to keep it as
%% an io:format though, since then if the rpc:multicall takes a while,
%% the user can still see what we're currently trying to do instead of
%% getting no feedback?
io:format("Setting config across the cluster~n", []),
{_, Down} = rpc:multicall(clique_nodes:nodes(),
?MODULE,
set_local_app_config,
[AppConfig],
60000),
case Down of
[] ->
[];
_ ->
Alert = io_lib:format("Failed to set config for: ~p~n", [Down]),
[clique_status:alert([clique_status:text(Alert)])]
end.
-spec set_app_config(proplist()) -> _.
set_app_config(AppConfig) ->
[application:set_env(App, Key, Val) || {App, Settings} <- AppConfig,
{Key, Val} <- Settings].

-spec config_flags() -> flagspecs().
config_flags() ->
Expand Down Expand Up @@ -428,4 +409,105 @@ schema_paths_test() ->
ok = file:delete("example.schema"),
?assertEqual([], schema_paths([Cwd])).

set_config_test_() ->
{setup,
fun set_config_test_setup/0,
fun set_config_test_teardown/1,
[
fun test_blacklisted_conf/0,
fun test_set_basic/0,
fun test_set_bad_flags/0,
fun test_set_all_flag/0,
fun test_set_node_flag/0,
fun test_set_bad_node/0,
fun test_set_config_callback/0,
fun test_set_callback_output/0
]}.

-define(SET_TEST_SCHEMA_FILE, "test.schema").

set_config_test_setup() ->
Schema = <<"{mapping, \"test.config\", \"clique.config_test\", [{datatype, integer}]}.">>,
{ok, Cwd} = file:get_cwd(),

?assertEqual(ok, clique_nodes:init()),
?assertEqual(true, clique_nodes:register(fun() -> [node()] end)),

?assertEqual(ok, file:write_file(?SET_TEST_SCHEMA_FILE, Schema)),
?assertEqual(ok, init()),
?assertEqual(ok, load_schema([Cwd])).

set_config_test_teardown(_) ->
_ = ets:delete(?config_table),
_ = ets:delete(?schema_table),
_ = ets:delete(?whitelist_table),
_ = ets:delete(?formatter_table),

file:delete(?SET_TEST_SCHEMA_FILE),

clique_nodes:teardown().

test_blacklisted_conf() ->
true = ets:delete_all_objects(?whitelist_table),
?assertEqual({error, {config_not_settable, ["test.config"]}}, set([{"test.config", "42"}], [])).

test_set_basic() ->
?assertEqual(ok, whitelist(["test.config"])),

Result = set([{"test.config", "42"}], []),
?assertNotMatch({error, _}, Result),
?assertEqual({ok, 42}, application:get_env(clique, config_test)).

test_set_bad_flags() ->
Result = set([{"test.config", "43"}], [{all, undefined}, {node, node()}]),
?assertMatch({error, {invalid_flag_combination, _}}, Result).

test_set_all_flag() ->
?assertEqual(ok, whitelist(["test.config"])),
Result = set([{"test.config", "44"}], [{all, undefined}]),
?assertNotMatch({error, _}, Result),
?assertEqual({ok, 44}, application:get_env(clique, config_test)).

test_set_node_flag() ->
?assertEqual(ok, whitelist(["test.config"])),
Result = set([{"test.config", "45"}], [{node, atom_to_list(node())}]),
?assertNotMatch({error, _}, Result),
?assertEqual({ok, 45}, application:get_env(clique, config_test)).

test_set_bad_node() ->
Result = set([{"test.config", "46"}], [{node, "bad_node@127.0.0.1"}]),
?assertEqual({error, bad_node}, Result).

test_set_config_callback() ->
true = ets:delete_all_objects(?config_table),
Callback = fun(Key, Val) ->
?assertEqual(["test", "config"], Key),
application:set_env(clique, config_test_x10, 10 * list_to_integer(Val)),
"Callback called"
end,
?MODULE:register(["test", "config"], Callback),
set([{"test.config", "47"}], []),
?assertEqual({ok, 47}, application:get_env(clique, config_test)),
?assertEqual({ok, 470}, application:get_env(clique, config_test_x10)).

test_set_callback_output() ->
true = ets:delete_all_objects(?config_table),
Callback = fun(_, _) -> "Done" end,
?MODULE:register(["test", "config"], Callback),

ExpectedText = <<"test.config set to \"48\"\nDone">>,
%% Slightly fragile way to test this, since we assume the internal representation
%% for clique text statuses won't change in the future. But this seems better than
%% any alternative I can think of, since two different iolists representing the
%% same data may or may not compare equal.
[{text, OutText}] = set([{"test.config", "48"}], []),
?assertEqual(ExpectedText, iolist_to_binary(OutText)),

ExpectedRow = [{"Node", node()},
{"Node Down/Unreachable", false},
{"Result", OutText}],
ExpectedTable = [clique_status:table([ExpectedRow])],
Result = set([{"test.config", "48"}], [{all, undefined}]),
?assertEqual(ExpectedTable, Result).

-endif.
Loading