OC-4956 OTP Structure

.gitignore

@@ -14,4 +14,7 @@ rel/opscode-pushy-server
 rel/pushy
 deps
 .eunit
-.pushy.plt
+
+/deps.plt
+
+/.concrete/

Makefile

@@ -1,106 +1,42 @@
-DEPS = deps/erlzmq deps/jiffy deps/gproc deps/ej \
-       deps/chef_authn deps/sqerl deps/mixer deps/lager \
-       deps/folsom deps/pushy_common
-
-ERLANG_APPS = erts kernel stdlib tools compiler syntax_tools runtime_tools\
-        crypto public_key ssl xmerl edoc eunit mnesia inets \
-        mnesia hipe syntax_tools runtime_tools edoc eunit asn1 gs webtool \
-        observer
-
-PLT_FILE = .pushy.plt
-
-all: dialyze eunit
-
-use_locked_config = $(wildcard USE_REBAR_LOCKED)
-ifeq ($(use_locked_config),USE_REBAR_LOCKED)
-  rebar_config = rebar.config.lock
+# This Makefile written by concrete
+#
+# {concrete_makefile_version, 1}
+#
+# Use this to override concrete's default dialyzer options of
+# -Wunderspecs
+# DIALYZER_OPTS = ...
+
+# Use this to point to the ebin directory of the current application.
+# Defaults to 'ebin' for "flat"file structures.  Nested application
+# structures might use something like "apps/MYAPP/ebin"
+#
+# Corresponds to dialyzer's '-r' flag
+DIALYZER_DIRS = apps/pushy/ebin
+
+# List dependencies that you do NOT want to be included in the
+# dialyzer PLT for the project here.  Typically, you would list a
+# dependency here if it isn't spec'd well and doesn't play nice with
+# dialyzer or otherwise mucks things up.
+#
+# DIALYZER_SKIP_DEPS = bad_dep_1 \
+#                      bad_dep_2
+
+# If you want to add dependencies to the default "all" target provided
+# by concrete, add them here (along with make rules to build them if needed)
+# ALL_HOOK = ...
+
+# Ensure that we get the proper goal for the default
+.DEFAULT_GOAL := all
+
+include lock_deps.mk
+
+concrete_rules_file = $(wildcard concrete.mk)
+ifeq ($(concrete_rules_file),concrete.mk)
+    include concrete.mk
 else
-  rebar_config = rebar.config
+    all:
+	@echo "ERROR: missing concrete.mk"
+	@echo "  run: concrete update"
 endif
-REBAR = rebar -C $(rebar_config)
-
-clean:
-	$(REBAR) clean
-
-allclean: depclean clean
-
-depclean:
-	@rm -rf deps
-
-compile: $(DEPS)
-	$(REBAR) compile
-
-compile_app:
-	$(REBAR) skip_deps=true compile
-
-$(PLT_FILE):
-	dialyzer -nn --output_plt $(PLT_FILE) --build_plt --apps $(ERLANG_APPS)
-
-plt_deps:
-	dialyzer --plt $(PLT_FILE) --output_plt $(PLT_FILE) --add_to_plt deps/*/ebin
-
-
-dialyze: compile $(PLT_FILE)
-	dialyzer -nn --plt $(PLT_FILE) -Wunmatched_returns -Werror_handling -r apps/pushy/ebin -I deps
-
-#dialyzer:
-#	@rm -rf apps/pushy/.eunit
-# Uncomment when stubbed functions in the FSM are complete
-# @dialyzer -Wrace_conditions -Wunderspecs -r apps --src
-
-$(DEPS):
-	$(REBAR) get-deps
-
-eunit: compile
-	$(REBAR) eunit skip_deps=true
-
-eunit_app: compile_app
-	$(REBAR) eunit apps=pushy skip_deps=true
-
-test: eunit
-
-tags:
-	@find src deps -name "*.[he]rl" -print | etags -
-
-rel: compile test rel/opscode-pushy-server
-rel/opscode-pushy-server:
-	@cd rel;$(REBAR) generate overlay_vars=db_vars.config
-
-relclean:
-	@rm -rf rel/opscode-pushy-server
-
-devrel: rel
-	@/bin/echo -n Symlinking deps and apps into release
-	@$(foreach lib,$(wildcard apps/* deps/*), /bin/echo -n .;rm -rf rel/opscode-pushy-server/lib/$(shell basename $(lib))-* \
-	   && ln -sf $(abspath $(lib)) rel/opscode-pushy-server/lib;)
-	@/bin/echo done.
-	@/bin/echo  Run \'make update\' to pick up changes in a running VM.
-
-update: compile
-	@cd rel/opscode-pushy-server;bin/opscode-pushy-server restart
-
-update_app: compile_app
-	@cd rel/opscode-pushy-server;bin/opscode-pushy-server restart
-
-distclean: relclean
-	@rm -rf deps
-	$(REBAR) clean
-
-BUMP ?= patch
-prepare_release: distclean unlocked_deps unlocked_compile update_locked_config rel
-	@echo 'release prepared, bumping $(BUMP) version'
-	@$(REBAR) bump-rel-version version=$(BUMP)
-
-unlocked_deps:
-	@echo 'Fetching deps as: rebar -C rebar.config'
-	@rebar -C rebar.config get-deps
-
-# When running the prepare_release target, we have to ensure that a
-# compile occurs using the unlocked rebar.config. If a dependency has
-# been removed, then using the locked version that contains the stale
-# dep will cause a compile error.
-unlocked_compile:
-	@rebar -C rebar.config compile
 
-update_locked_config:
-	@rebar lock-deps ignore=meck skip_deps=true
+include release.mk

apps/pushy/include/pushy_types.hrl

@@ -13,3 +13,11 @@
 -type node_availability() :: available |
                              unavailable.
 
+-type incarnation_id() :: binary().
+
+%% TODO: this seems as though it could be tightened up
+%%
+%% TODO: The binary expression is the same as object_id() in
+%% pushy_sql.hrl.  We need to do a bit of type refactoring so that we
+%% can directly use that type here, instead of duplicating.
+-type job_id() :: <<_:256>> | null | invalid_job_id.

apps/pushy/include/pushy_wm.hrl

@@ -15,14 +15,15 @@
                        % TODO: probably want to split this into specific states, instead of this
                        % catch-all, but right now authentication requires the above two things,
                        % and config + job endpoints (respectively) need the following:
-                       node_name :: string(),
+                       node_name :: binary(),
                        % TODO: can't use #pushy_job{} here without some minor re-jiggering elsewhere;
                        % not everything that includes this includes pushy_sql.hrl
                        pushy_job :: tuple(),
                        incarnation_id :: binary(),
                        requestor :: binary(),
 
                        %% Authz ID of the requestor
+                       %% TODO: Why isn't this just an embedded #pushy_principal{} record instead?
                        requestor_id :: binary(),
                        requestor_type :: pushy_requestor_type(),
                        requestor_key :: any()

apps/pushy/src/pushy.hrl

@@ -7,7 +7,7 @@
 
 -record(pushy_state, {
           ctx :: any(),
-          incarnation_id :: binary()
+          incarnation_id :: incarnation_id()
          }).
 
 -define(PUSHY_BROKER_OUT, "inproc://pushy_broker_out").

apps/pushy/src/pushy_check_groups.erl

@@ -71,8 +71,5 @@ check_subgroups(Name, Type, OrgName, [Group|OtherGroups]) ->
             check_subgroups(Name, Type, OrgName, OtherGroups)
     end.
 
--spec field_for_type(pushy_requestor_type()) -> binary().
-field_for_type(user) ->
-    <<"users">>;
-field_for_type(client) ->
-    <<"clients">>.
+field_for_type(user)   -> <<"users">>;
+field_for_type(client) -> <<"clients">>.

apps/pushy/src/pushy_command_switch.erl

@@ -51,7 +51,10 @@
         {r_sock,
          s_sock}).
 
--type addressed_message() :: [binary()]. % TODO Improve; it might be worth turning this into a tuple for better specificity.
+%% TODO: some refactoring around this seems necessary.  First, it seems that this is
+%% actually multiple messages (see pushy_job_state:do_send/3).  Turning it into a tuple for
+%% better specificity might also be a good idea.
+-type addressed_message() :: nonempty_list(binary()).
 
 %% ------------------------------------------------------------------
 %% API Function Definitions

apps/pushy/src/pushy_http_common.erl

@@ -10,7 +10,7 @@
 
 fetch_authenticated(Path) ->
     {ok, Key} = chef_keyring:get_key(pivotal),
-    Headers = chef_authn:sign_request(Key, <<"">>, "pivotal",
+    Headers = chef_authn:sign_request(Key, <<"pivotal">>,
                                       <<"GET">>, now, list_to_binary(Path)),
     FullHeaders = [{"Accept", "application/json"}|Headers],
     fetch_authenticated(Path, FullHeaders).
@@ -28,7 +28,7 @@ fetch_authenticated(Path, Headers) ->
     end.
 
 url(Path) ->
-    {ok, ErchefHost} = application:get_env(pushy, erchef_root_url),
+    ErchefHost = envy:get(pushy, erchef_root_url, string),
     ErchefHost ++ Path.
 
 check_http_response(Code, Headers, Body) ->

apps/pushy/src/pushy_job_monitor.erl

@@ -58,15 +58,15 @@ init(State) ->
 handle_call({is_monitored, Pid}, _From, #state{jobs = Jobs} =State) ->
     {reply, dict:is_key(Pid, Jobs), State};
 handle_call(Msg, _From, State) ->
-    lager:warn("Unknown message handle_call: ~p~n", [Msg]),
+    lager:warning("Unknown message handle_call: ~p~n", [Msg]),
     {reply, ok, State}.
 
 handle_cast({monitor, JobId, Pid}, #state{jobs = Jobs} = State) ->
     erlang:monitor(process, Pid),
     State1 = State#state{jobs = dict:store(Pid, JobId, Jobs)},
     {noreply, State1};
 handle_cast(Msg, State) ->
-    lager:warn("Unknown message handle_cast: ~p~n", [Msg]),
+    lager:warning("Unknown message handle_cast: ~p~n", [Msg]),
     {noreply, State}.
 
 %% If we shutdown normally do nothing
@@ -86,7 +86,7 @@ handle_info({'DOWN', _Ref, process, Pid, _Reason},
     end,
     {noreply, State#state{jobs=dict:erase(Pid, Jobs)}};
 handle_info(Msg, State) ->
-    lager:warn("Unknown message handle_info: ~p~n", [Msg]),
+    lager:warning("Unknown message handle_info: ~p~n", [Msg]),
     {noreply, State}.
 
 terminate(_Reason, _State) ->
@@ -113,7 +113,7 @@ mark_as_crashed(#pushy_job{id = JobId,
         {error, Error}
   end.
 
--spec nodes_to_rehab(JobNodes :: [#pushy_job_node{}]) -> ok | {error, term()}.
+-spec nodes_to_rehab(JobNodes :: [#pushy_job_node{}]) -> ok.
 nodes_to_rehab([]) ->
     ok;
 nodes_to_rehab([#pushy_job_node{org_id = OrgId,

apps/pushy/src/pushy_job_state.erl

@@ -157,9 +157,9 @@ handle_event(Event, StateName, State) ->
     {next_state, StateName, State}.
 
 -spec handle_sync_event(any(), any(), job_status(), #state{}) ->
-        {'next_state', job_status(), #state{}}|
-        {'reply', 'ok', job_status(), #state{}} |
-        {'stop', 'shutdown', 'ok', #state{}}.
+                               {'next_state', job_status(), #state{}}|
+                               {'reply', #pushy_job{}, job_status(), #state{}} |
+                               {'stop', 'shutdown', 'ok', #state{}}.
 handle_sync_event(get_job_status, _From, StateName,
         #state{job = Job, job_nodes = JobNodes} = State) ->
     JobNodesList = [ JobNode || {_,JobNode} <- dict:to_list(JobNodes) ],

apps/pushy/src/pushy_job_state_sup.erl

@@ -76,7 +76,7 @@ register_process(JobId) ->
 %% Internal functions
 %% ------------------------------------------------------------------
 
--spec mark_incomplete_job_nodes_as_crashed() -> ok | {error, term()}.
+-spec mark_incomplete_job_nodes_as_crashed() -> ok | {error, no_connections | {_, _}}.
 %% Find running job nodes associated with crashed jobs. Mark them as crashed in the db.
 mark_incomplete_job_nodes_as_crashed() ->
     case pushy_sql:fetch_incomplete_job_nodes() of
@@ -94,7 +94,7 @@ update_job_node([Node | Nodes]) ->
     update_job_node(Nodes).
 
 
--spec mark_incomplete_jobs_as_crashed() -> ok | {error, term()}.
+-spec mark_incomplete_jobs_as_crashed() -> ok | {error, no_connections | {_,_}}.
 mark_incomplete_jobs_as_crashed() ->
     case pushy_sql:fetch_incomplete_jobs() of
         {ok, Jobs} ->

apps/pushy/src/pushy_key_manager.erl

@@ -28,14 +28,16 @@
 -define(TABLE, pushy_key_manager).
 -define(KEYTYPE, hmac_sha256).
 
--type key() :: {atom(), binary()}.
+-type key() :: {?KEYTYPE, binary()}.
 
 -spec init() -> ok.
 init() ->
     _Tid = ets:new(?TABLE, [set,public,named_table, {read_concurrency, true}]),
     ok.
 
 %% This exists to make eunit tests less painful to write.
+
+%% TODO: conditionally compile and export this only when we're doing testing
 -spec stop() -> ok.
 stop() ->
     true = ets:delete(?TABLE),
@@ -44,7 +46,8 @@ stop() ->
 %%%
 %%% Key descriptor: {algorithm:atom, key:binary}
 %%%
--spec generate_key(atom()) -> key().
+%%
+%% TODO: as currently coded, this function's argument is completely superfluous
 generate_key(hmac_sha256) ->
     Key = pushy_util:rand_bytes(erlang:trunc(256/8)),
     {hmac_sha256, Key}.

apps/pushy/src/pushy_node_state.erl

@@ -48,7 +48,7 @@
                 sequence_no = 1       :: pos_integer(),
                 availability          :: node_availability(),
                 watchers = [],
-                incarnation_id,
+                incarnation_id        :: incarnation_id(),
                 state_timer
                }).
 
@@ -393,7 +393,11 @@ process_message(#state{node_ref=NodeRef, node_addr=Address} = State, #pushy_mess
             send_node_event(State, JobId, NodeRef, Type)
     end.
 
--spec send_node_event(#state{}, any(), any(), any(), node_event()) -> #state{}.
+-spec send_node_event(#state{},
+                      job_id(),
+                      node_ref(),
+                      incarnation_id(),
+                      node_event()) -> #state{}.
 send_node_event(State, JobId, NodeRef, IncarnationId, heartbeat) ->
     lager:debug("Received heartbeat for node ~p with job id ~p", [NodeRef, JobId]),
     case JobId /= null andalso pushy_job_state_sup:get_process(JobId) == not_found of
@@ -405,7 +409,10 @@ send_node_event(State, JobId, NodeRef, IncarnationId, heartbeat) ->
     send_info(self(), {heartbeat, IncarnationId}),
     State.
 
--spec send_node_event(#state{}, any(), any(), node_event()) -> #state{}.
+-spec send_node_event(#state{},
+                      job_id(),
+                      node_ref(),
+                      node_event()) -> #state{}.
 send_node_event(State, JobId, NodeRef, aborted = Msg) ->
     gen_fsm:send_event(self(), aborted),
     if
@@ -441,11 +448,11 @@ key_fetch(Method, EJson) ->
     NodeRef = get_node_ref(EJson),
     get_key_for_method(Method, NodeRef).
 
--spec do_send(#state{}, json_term()) -> #state{}.
+-spec do_send(#state{}, ej:json_term()) -> #state{}.
 do_send(State, Message) ->
     do_send(State, hmac_sha256, Message).
 
--spec do_send(#state{}, atom(), json_term()) -> #state{}.
+-spec do_send(#state{}, pushy_signing_method(), ej:json_term()) -> #state{}.
 do_send(#state{node_addr=NodeAddr, node_ref=NodeRef, sequence_no = SeqNo} = State,
         Method, Message) ->
     %% Normalize msg by adding standard fields.
@@ -471,6 +478,8 @@ message_type_to_atom(_) -> unknown.
 %%
 %% TODO: This should be revisited when we tackle OC-5328 (handle ill formed packets set to pushy)
 %%
+%% TODO: Revisit this spec, w/r/t job_id().
+-spec extract_job_id(ej:json_object()) -> job_id().
 extract_job_id(Data) ->
     case ej:get({<<"job_id">>}, Data) of
         <<"null">> ->