Add online flag. Get the running broker to read its own plugins file rather than tell it which plugins to run. This necessitates various bug fixes to rabbit_plugins.

3 files changed, 108 insertions, 63 deletions

diff --git a/docs/rabbitmq-plugins.1.xml b/docs/rabbitmq-plugins.1.xml
index d3268af3..e891969f 100644
--- a/docs/rabbitmq-plugins.1.xml
+++ b/docs/rabbitmq-plugins.1.xml
@@ -133,7 +133,7 @@
       </varlistentry>
 
       <varlistentry>
-        <term><cmdsynopsis><command>enable</command> <arg choice="opt">--offline</arg> <arg choice="req"><replaceable>plugin</replaceable> ...</arg></cmdsynopsis></term>
+        <term><cmdsynopsis><command>enable</command> <arg choice="opt">--offline</arg> <arg choice="opt">--online</arg> <arg choice="req"><replaceable>plugin</replaceable> ...</arg></cmdsynopsis></term>
         <listitem>
           <variablelist>
             <varlistentry>
@@ -141,6 +141,10 @@
               <listitem><para>Just modify the enabled plugins file.</para></listitem>
             </varlistentry>
             <varlistentry>
+              <term>--online</term>
+              <listitem><para>Treat failure to connect to the running broker as fatal.</para></listitem>
+            </varlistentry>
+            <varlistentry>
               <term>plugin</term>
               <listitem><para>One or more plugins to enable.</para></listitem>
             </varlistentry>
@@ -148,8 +152,12 @@
           <para>
             Enables the specified plugins and all their
             dependencies. This will update the enabled plugins file
-            and then connect to the broker and ensure it is running
-            all enabled plugins.
+            and then attempt to connect to the broker and ensure it is
+            running all enabled plugins. By default if it is not
+            possible to connect to the running broker (for example if
+            it is stopped) then a warning is displayed. Specify
+            <command>--online</command> or
+            <command>--offline</command> to change this.
           </para>
 
           <para role="example-prefix">For example:</para>
@@ -163,7 +171,7 @@
       </varlistentry>
 
       <varlistentry>
-        <term><cmdsynopsis><command>disable</command> <arg choice="opt">--offline</arg> <arg choice="req"><replaceable>plugin</replaceable> ...</arg></cmdsynopsis></term>
+        <term><cmdsynopsis><command>disable</command> <arg choice="opt">--offline</arg> <arg choice="opt">--online</arg> <arg choice="req"><replaceable>plugin</replaceable> ...</arg></cmdsynopsis></term>
         <listitem>
           <variablelist>
             <varlistentry>
@@ -171,6 +179,10 @@
               <listitem><para>Just modify the enabled plugins file.</para></listitem>
             </varlistentry>
             <varlistentry>
+              <term>--online</term>
+              <listitem><para>Treat failure to connect to the running broker as fatal.</para></listitem>
+            </varlistentry>
+            <varlistentry>
               <term>plugin</term>
               <listitem><para>One or more plugins to disable.</para></listitem>
             </varlistentry>
@@ -178,8 +190,12 @@
           <para>
             Disables the specified plugins and all their
             dependencies. This will update the enabled plugins file
-            and then connect to the broker and ensure it is running
-            all enabled plugins.
+            and then attempt to connect to the broker and ensure it is
+            running all enabled plugins. By default if it is not
+            possible to connect to the running broker (for example if
+            it is stopped) then a warning is displayed. Specify
+            <command>--online</command> or
+            <command>--offline</command> to change this.
           </para>
 
           <para role="example-prefix">For example:</para>
diff --git a/src/rabbit_plugins.erl b/src/rabbit_plugins.erl
index 39639b6d..81f8c274 100644
--- a/src/rabbit_plugins.erl
+++ b/src/rabbit_plugins.erl
@@ -32,25 +32,34 @@
 -spec(read_enabled/1 :: (file:filename()) -> [plugin_name()]).
 -spec(dependencies/3 :: (boolean(), [plugin_name()], [#plugin{}]) ->
                              [plugin_name()]).
--spec(ensure/1  :: ([plugin_name()]) -> {'ok', [atom()], [atom()]}).
+-spec(ensure/1  :: (string()) -> {'ok', [atom()], [atom()]} | {error, any()}).
 -endif.
 
 %%----------------------------------------------------------------------------
 
-ensure(Wanted) ->
-    Current = active(),
-    Start = Wanted -- Current,
-    Stop = Current -- Wanted,
-    prepare_plugins(Start),
-    rabbit:start_apps(Start),
-    %% We need sync_notify here since mgmt will attempt to look at all
-    %% the modules for the disabled plugins - if they are unloaded
-    %% that won't work.
-    ok = rabbit_event:notify(plugins_changed, [{enabled,  Start},
-                                               {disabled, Stop}]),
-    rabbit:stop_apps(Stop),
-    clean_plugins(Stop),
-    {ok, Start, Stop}.
+ensure(FileJustChanged) ->
+    {ok, OurFile} = application:get_env(rabbit, enabled_plugins_file),
+    case OurFile of
+        FileJustChanged ->
+            {ok, Dir} = application:get_env(rabbit, plugins_dir),
+            Enabled = read_enabled(OurFile),
+            Wanted = dependencies(false, Enabled, list(Dir)),
+            prepare_plugins(Enabled),
+            Current = active(),
+            Start = Wanted -- Current,
+            Stop = Current -- Wanted,
+            rabbit:start_apps(Start),
+            %% We need sync_notify here since mgmt will attempt to look at all
+            %% the modules for the disabled plugins - if they are unloaded
+            %% that won't work.
+            ok = rabbit_event:notify(plugins_changed, [{enabled,  Start},
+                                                       {disabled, Stop}]),
+            rabbit:stop_apps(Stop),
+            clean_plugins(Stop),
+            {ok, Start, Stop};
+        _ ->
+            {error, {enabled_plugins_mismatch, FileJustChanged, OurFile}}
+    end.
 
 %% @doc Prepares the file system and installs all enabled plugins.
 setup() ->
@@ -70,7 +79,7 @@ setup() ->
 %% @doc Lists the plugins which are currently running.
 active() ->
     {ok, ExpandDir} = application:get_env(rabbit, plugins_expand_dir),
-    InstalledPlugins = [ P#plugin.name || P <- list(ExpandDir) ],
+    InstalledPlugins = plugin_names(list(ExpandDir)),
     [App || {App, _, _} <- rabbit_misc:which_applications(),
             lists:member(App, InstalledPlugins)].
 
@@ -91,7 +100,7 @@ list(PluginsDir) ->
         _  -> error_logger:warning_msg(
                 "Problem reading some plugins: ~p~n", [Problems])
     end,
-    Plugins.
+    ensure_dependencies(Plugins).
 
 %% @doc Read the list of enabled plugins from the supplied term file.
 read_enabled(PluginsFile) ->
@@ -112,13 +121,8 @@ dependencies(Reverse, Sources, AllPlugins) ->
     {ok, G} = rabbit_misc:build_acyclic_graph(
                 fun ({App, _Deps}) -> [{App, App}] end,
                 fun ({App,  Deps}) -> [{App, Dep} || Dep <- Deps] end,
-                lists:ukeysort(
-                  1, [{Name, Deps} ||
-                         #plugin{name         = Name,
-                                 dependencies = Deps} <- AllPlugins] ++
-                      [{Dep,   []} ||
-                          #plugin{dependencies = Deps} <- AllPlugins,
-                          Dep                          <- Deps])),
+                [{Name, Deps} || #plugin{name         = Name,
+                                         dependencies = Deps} <- AllPlugins]),
     Dests = case Reverse of
                 false -> digraph_utils:reachable(Sources, G);
                 true  -> digraph_utils:reaching(Sources, G)
@@ -126,6 +130,28 @@ dependencies(Reverse, Sources, AllPlugins) ->
     true = digraph:delete(G),
     Dests.
 
+%% Make sure we don't list OTP apps in here, and also that we create
+%% fake plugins for missing dependencies.
+ensure_dependencies(Plugins) ->
+    Names = plugin_names(Plugins),
+    NotThere = [Dep || #plugin{dependencies = Deps} <- Plugins,
+                       Dep                          <- Deps,
+                       not lists:member(Dep, Names)],
+    {OTP, Missing} = lists:partition(fun is_loadable/1, NotThere),
+    Plugins1 = [P#plugin{dependencies = Deps -- OTP}
+                || P = #plugin{dependencies = Deps} <- Plugins],
+    Fake = [#plugin{name         = Name,
+                    dependencies = []}|| Name <- Missing],
+    Plugins1 ++ Fake.
+
+is_loadable(App) ->
+    case application:load(App) of
+        {error, {already_loaded, _}} -> true;
+        ok                           -> application:unload(App),
+                                        true;
+        _                            -> false
+   end.
+
 %%----------------------------------------------------------------------------
 
 prepare_plugins(Enabled) ->
@@ -206,8 +232,7 @@ plugin_info(Base, {app, App0}) ->
 mkplugin(Name, Props, Type, Location) ->
     Version = proplists:get_value(vsn, Props, "0"),
     Description = proplists:get_value(description, Props, ""),
-    Dependencies =
-        filter_applications(proplists:get_value(applications, Props, [])),
+    Dependencies = proplists:get_value(applications, Props, []),
     #plugin{name = Name, version = Version, description = Description,
             dependencies = Dependencies, location = Location, type = Type}.
 
@@ -240,18 +265,6 @@ parse_binary(Bin) ->
         Err -> {error, {invalid_app, Err}}
     end.
 
-filter_applications(Applications) ->
-    [Application || Application <- Applications,
-                    not is_available_app(Application)].
-
-is_available_app(Application) ->
-    case application:load(Application) of
-        {error, {already_loaded, _}} -> true;
-        ok                           -> application:unload(Application),
-                                        true;
-        _                            -> false
-    end.
-
 plugin_names(Plugins) ->
     [Name || #plugin{name = Name} <- Plugins].
 
diff --git a/src/rabbit_plugins_main.erl b/src/rabbit_plugins_main.erl
index 981fc649..b7bc9ce6 100644
--- a/src/rabbit_plugins_main.erl
+++ b/src/rabbit_plugins_main.erl
@@ -26,6 +26,7 @@
 -define(ENABLED_OPT, "-E").
 -define(ENABLED_ALL_OPT, "-e").
 -define(OFFLINE_OPT, "--offline").
+-define(ONLINE_OPT, "--online").
 
 -define(NODE_DEF(Node), {?NODE_OPT, {option, Node}}).
 -define(VERBOSE_DEF, {?VERBOSE_OPT, flag}).
@@ -33,6 +34,7 @@
 -define(ENABLED_DEF, {?ENABLED_OPT, flag}).
 -define(ENABLED_ALL_DEF, {?ENABLED_ALL_OPT, flag}).
 -define(OFFLINE_DEF, {?OFFLINE_OPT, flag}).
+-define(ONLINE_DEF, {?ONLINE_OPT, flag}).
 
 -define(RPC_TIMEOUT, infinity).
 
@@ -40,8 +42,8 @@
 
 -define(COMMANDS,
         [{list, [?VERBOSE_DEF, ?MINIMAL_DEF, ?ENABLED_DEF, ?ENABLED_ALL_DEF]},
-         {enable, [?OFFLINE_DEF]},
-         {disable, [?OFFLINE_DEF]},
+         {enable, [?OFFLINE_DEF, ?ONLINE_DEF]},
+         {disable, [?OFFLINE_DEF, ?ONLINE_DEF]},
          {sync, []}]).
 
 %%----------------------------------------------------------------------------
@@ -153,7 +155,8 @@ action(enable, Node, ToEnable0, Opts, PluginsFile, PluginsDir) ->
         _  -> print_list("The following plugins have been enabled:",
                          NewImplicitlyEnabled -- ImplicitlyEnabled)
     end,
-    action_change(Opts, Node, ImplicitlyEnabled, NewImplicitlyEnabled);
+    action_change(
+      Opts, Node, ImplicitlyEnabled, NewImplicitlyEnabled, PluginsFile);
 
 action(disable, Node, ToDisable0, Opts, PluginsFile, PluginsDir) ->
     case ToDisable0 of
@@ -180,13 +183,11 @@ action(disable, Node, ToDisable0, Opts, PluginsFile, PluginsDir) ->
                             ImplicitlyEnabled -- NewImplicitlyEnabled),
                  write_enabled_plugins(PluginsFile, NewEnabled)
     end,
-    action_change(Opts, Node, ImplicitlyEnabled, NewImplicitlyEnabled);
+    action_change(
+      Opts, Node, ImplicitlyEnabled, NewImplicitlyEnabled, PluginsFile);
 
-action(sync, Node, X, Opts, PluginsFile, PluginsDir) ->
-    AllPlugins = rabbit_plugins:list(PluginsDir),
-    Enabled = rabbit_plugins:read_enabled(PluginsFile),
-    ImplicitlyEnabled = rabbit_plugins:dependencies(false, Enabled, AllPlugins),
-    action_change(Opts, Node, ImplicitlyEnabled, ImplicitlyEnabled).
+action(sync, Node, [], _Opts, PluginsFile, _PluginsDir) ->
+    sync(Node, true, PluginsFile).
 
 %%----------------------------------------------------------------------------
 
@@ -312,21 +313,23 @@ write_enabled_plugins(PluginsFile, Plugins) ->
                                           PluginsFile, Reason}})
     end.
 
-action_change(Opts, Node, Old, New) ->
-    action_change0(proplists:get_bool(?OFFLINE_OPT, Opts), Node, Old, New).
+action_change(Opts, Node, Old, New, PluginsFile) ->
+    action_change0(proplists:get_bool(?OFFLINE_OPT, Opts),
+                   proplists:get_bool(?ONLINE_OPT, Opts),
+                   Node, Old, New, PluginsFile).
 
-action_change0(true, _Node, Same, Same) ->
+action_change0(true, _Online, _Node, Same, Same, _PluginsFile) ->
     %% Definitely nothing to do
     ok;
-action_change0(true, _Node, _Old, _New) ->
+action_change0(true, _Online, _Node, _Old, _New, _PluginsFile) ->
     io:format("Offline change; changes will take effect at broker restart.~n");
-action_change0(false, Node, _Old, New) ->
-    %% Don't care what the Old was in the plugins file, that might not
-    %% match what the server is running - so tell it to ensure we are
-    %% running the right apps even if "nothing has changed".
-    rpc_call(Node, rabbit_plugins, ensure, [New]).
+action_change0(false, Online, Node, _Old, _New, PluginsFile) ->
+    sync(Node, Online, PluginsFile).
 
-rpc_call(Node, Mod, Fun, Args) ->
+sync(Node, ForceOnline, PluginsFile) ->
+    rpc_call(Node, ForceOnline, rabbit_plugins, ensure, [PluginsFile]).
+
+rpc_call(Node, Online, Mod, Fun, Args) ->
     io:format("Applying plugin configuration to ~s...", [Node]),
     case rpc:call(Node, Mod, Fun, Args) of
         {ok, [], []} ->
@@ -338,6 +341,19 @@ rpc_call(Node, Mod, Fun, Args) ->
         {ok, Start, Stop} ->
             io:format(" stopped ~b plugin~s and started ~b plugin~s.~n",
                       [length(Stop), plur(Stop), length(Start), plur(Start)]);
+        {badrpc, nodedown} = Error ->
+            io:format(" failed.~n", []),
+            case Online of
+                true  -> Error;
+                false -> io:format(
+                           " * Could not contact node ~s.~n"
+                           " * Changes will take effect at broker restart.~n"
+                           " * Specify --online for diagnostics and to treat "
+                           "this as a failure.~n"
+                           " * Specify --offline to disable changes to running "
+                           "broker.~n",
+                           [Node])
+            end;
         {badrpc, _} = Error ->
             io:format(" failed.~n", []),
             Error