Merge pull request #1556 from rabbitmq/vhost-sup-race

Do not try to start a vhost supervisors on not fully booted nodes.

3 files changed, 39 insertions, 5 deletions

diff --git a/src/rabbit.erl b/src/rabbit.erl
index 37b33bd8a4..cde1d0ad07 100644
--- a/src/rabbit.erl
+++ b/src/rabbit.erl
@@ -27,6 +27,7 @@
 -export([start/2, stop/1, prep_stop/1]).
 -export([start_apps/1, start_apps/2, stop_apps/1]).
 -export([log_locations/0, config_files/0, decrypt_config/2]). %% for testing and mgmt-agent
+-export([is_booted/1]).
 
 -ifdef(TEST).
 
@@ -745,6 +746,13 @@ is_running() -> is_running(node()).
 
 is_running(Node) -> rabbit_nodes:is_process_running(Node, rabbit).
 
+is_booted(Node) ->
+    case is_booting(Node) of
+        false ->
+            is_running(Node);
+        _ -> false
+    end.
+
 environment() ->
     %% The timeout value is twice that of gen_server:call/2.
     [{A, environment(A)} ||
diff --git a/src/rabbit_vhost_sup_sup.erl b/src/rabbit_vhost_sup_sup.erl
index 558648400a..3ce3201d6b 100644
--- a/src/rabbit_vhost_sup_sup.erl
+++ b/src/rabbit_vhost_sup_sup.erl
@@ -60,14 +60,16 @@ init([]) ->
             [rabbit_vhost_sup_wrapper, rabbit_vhost_sup]}]}}.
 
 start_on_all_nodes(VHost) ->
-    NodesStart = [ {Node, start_vhost(VHost, Node)}
-                   || Node <- rabbit_nodes:all_running() ],
-    Failures = lists:filter(fun
+    %% Do not try to start a vhost on booting peer nodes
+    AllBooted = [Node || Node <- rabbit_nodes:all_running(), rabbit:is_booted(Node)],
+    Nodes     = [node() | AllBooted],
+    Results   = [{Node, start_vhost(VHost, Node)} || Node <- Nodes],
+    Failures  = lists:filter(fun
                                ({_, {ok, _}}) -> false;
                                ({_, {error, {already_started, _}}}) -> false;
                                (_) -> true
                             end,
-                            NodesStart),
+                            Results),
     case Failures of
         []     -> ok;
         Errors -> {error, {failed_to_start_vhost_on_nodes, Errors}}
diff --git a/test/clustering_management_SUITE.erl b/test/clustering_management_SUITE.erl
index e04fff2182..e9f52de3c8 100644
--- a/test/clustering_management_SUITE.erl
+++ b/test/clustering_management_SUITE.erl
@@ -53,7 +53,8 @@ groups() ->
               forget_offline_removes_things,
               force_boot,
               status_with_alarm,
-              wait_fails_when_cluster_fails
+              wait_fails_when_cluster_fails,
+              concurrent_default_data_creation
             ]},
           {cluster_size_4, [], [
               forget_promotes_offline_slave
@@ -636,6 +637,29 @@ wait_fails_when_cluster_fails(Config) ->
     {error, _, _} = rabbit_ct_broker_helpers:rabbitmqctl(Config, Rabbit,
       ["wait", RabbitPidFile]).
 
+concurrent_default_data_creation(Config) ->
+    [Rabbit, Hare] = rabbit_ct_broker_helpers:get_node_configs(Config,
+      nodename),
+    %% Run multiple times to detect a race.
+    %% This test simulates concurrent initialisation of several key node DB tables.
+    %% Since this is node-local state, in practice this can only
+    %% happen when a new cluster is formed and two nodes are booting
+    %% at roughly the same time (say, within a couple of ms from each other).
+    [concurrent_default_data_creation1(Rabbit, Hare) || _ <- lists:seq(1, 20)].
+
+concurrent_default_data_creation1(Rabbit, Hare) ->
+    %% Clear default data.
+    [{atomic, ok} = rpc:call(Rabbit, mnesia, clear_table, [Tab])
+     || Tab <- [rabbit_user, rabbit_user_permission, rabbit_vhost]],
+    %% Stop both nodes
+    [ok = rpc:call(Node, rabbit, stop, [])  || Node <- [Rabbit, Hare]],
+    %% Start nodes in parallel
+    [spawn(fun() -> rpc:call(Node, rabbit, start, []) end)
+     || Node <- [Rabbit, Hare]],
+    %% Verify both nodes are started successfully
+    [ok = rpc:call(Node, rabbit, await_startup, [Node]) || Node <- [Rabbit, Hare]],
+    [{ok, _Pid} = rpc:call(Node, rabbit_vhost_sup_sup, get_vhost_sup, [<<"/">>]) || Node <- [Rabbit, Hare]].
+
 %% ----------------------------------------------------------------------------
 %% Internal utils
 %% ----------------------------------------------------------------------------