Merge pull request #11706 from rabbitmq/md/khepri-minority-errors/rab…

…bit_db_vhost Handle timeouts possible in Khepri minority in `rabbit_db_vhost`
rabbitmq · Jul 25, 2024 · 29251a0 · 29251a0
2 parents 5a56e32 + 8399450
commit 29251a0
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 12 deletions.
diff --git a/deps/rabbit/src/rabbit_db_vhost.erl b/deps/rabbit/src/rabbit_db_vhost.erl
@@ -62,7 +62,7 @@
       VHostName :: vhost:name(),
       Limits :: vhost:limits(),
       Metadata :: vhost:metadata(),
-      Ret :: {existing | new, VHost},
+      Ret :: {existing | new, VHost} | no_return(),
       VHost :: vhost:vhost().
 %% @doc Writes a virtual host record if it doesn't exist already or returns
 %% the existing one.
@@ -117,7 +117,9 @@ create_or_get_in_khepri(VHostName, VHost) ->
 -spec merge_metadata(VHostName, Metadata) -> Ret when
       VHostName :: vhost:name(),
       Metadata :: vhost:metadata(),
-      Ret :: {ok, VHost} | {error, {no_such_vhost, VHostName}},
+      Ret :: {ok, VHost} |
+             {error, {no_such_vhost, VHostName}} |
+             rabbit_khepri:timeout_error(),
       VHost :: vhost:vhost().
 %% @doc Updates the metadata of an existing virtual host record.
 %%
@@ -188,7 +190,7 @@ merge_metadata_in_khepri(VHostName, Metadata) ->
 -spec set_tags(VHostName, Tags) -> VHost when
       VHostName :: vhost:name(),
       Tags :: [vhost:tag() | binary() | string()],
-      VHost :: vhost:vhost().
+      VHost :: vhost:vhost() | no_return().
 %% @doc Sets the tags of an existing virtual host record.
 %%
 %% @returns the updated virtual host record if the record existed and the
@@ -347,7 +349,7 @@ list_in_khepri() ->
 -spec update(VHostName, UpdateFun) -> VHost when
       VHostName :: vhost:name(),
       UpdateFun :: fun((VHost) -> VHost),
-      VHost :: vhost:vhost().
+      VHost :: vhost:vhost() | no_return().
 %% @doc Updates an existing virtual host record using the result of
 %% `UpdateFun'.
 %%
@@ -439,9 +441,10 @@ with_fun_in_khepri_tx(VHostName, Thunk) ->
 %% delete().
 %% -------------------------------------------------------------------
 
--spec delete(VHostName) -> Existed when
+-spec delete(VHostName) -> Ret when
       VHostName :: vhost:name(),
-      Existed :: boolean().
+      Existed :: boolean(),
+      Ret :: Existed | rabbit_khepri:timeout_error().
 %% @doc Deletes a virtual host record from the database.
 %%
 %% @returns a boolean indicating if the vhost existed or not. It throws an
@@ -468,7 +471,7 @@ delete_in_khepri(VHostName) ->
     case rabbit_khepri:delete_or_fail(Path) of
         ok -> true;
         {error, {node_not_found, _}} -> false;
-        _ -> false
+        {error, _} = Err -> Err
     end.
 
 %% -------------------------------------------------------------------

diff --git a/deps/rabbit/src/rabbit_definitions.erl b/deps/rabbit/src/rabbit_definitions.erl
@@ -773,7 +773,7 @@ add_policy(VHost, Param, Username) ->
                              exit(rabbit_data_coercion:to_binary(rabbit_misc:escape_html_tags(E ++ S)))
     end.
 
--spec add_vhost(map(), rabbit_types:username()) -> ok.
+-spec add_vhost(map(), rabbit_types:username()) -> ok | no_return().
 
 add_vhost(VHost, ActingUser) ->
     Name             = maps:get(name, VHost, undefined),
@@ -783,7 +783,12 @@ add_vhost(VHost, ActingUser) ->
     Tags             = maps:get(tags, VHost, maps:get(tags, Metadata, [])),
     DefaultQueueType = maps:get(default_queue_type, Metadata, undefined),
 
-    rabbit_vhost:put_vhost(Name, Description, Tags, DefaultQueueType, IsTracingEnabled, ActingUser).
+    case rabbit_vhost:put_vhost(Name, Description, Tags, DefaultQueueType, IsTracingEnabled, ActingUser) of
+        ok ->
+            ok;
+        {error, _} = Err ->
+            throw(Err)
+    end.
 
 add_permission(Permission, ActingUser) ->
     rabbit_auth_backend_internal:set_permissions(maps:get(user,      Permission, undefined),

diff --git a/deps/rabbit/src/rabbit_vhost.erl b/deps/rabbit/src/rabbit_vhost.erl
@@ -311,7 +311,9 @@ delete(VHost, ActingUser) ->
                         [{name, VHost},
                          {user_who_performed_action, ActingUser}]);
              false ->
-                 {error, {no_such_vhost, VHost}}
+                 {error, {no_such_vhost, VHost}};
+             {error, _} = Err ->
+                 Err
          end,
     %% After vhost was deleted from the database, we try to stop vhost
     %% supervisors on all the nodes.

diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_vhost.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_vhost.erl
@@ -92,8 +92,22 @@ accept_content(ReqData0, Context = #context{user = #user{username = Username}})
 
 delete_resource(ReqData, Context = #context{user = #user{username = Username}}) ->
     VHost = id(ReqData),
-    _ = rabbit_vhost:delete(VHost, Username),
-    {true, ReqData, Context}.
+    case rabbit_vhost:delete(VHost, Username) of
+        ok ->
+            {true, ReqData, Context};
+        {error, timeout} ->
+            rabbit_mgmt_util:internal_server_error(
+              timeout,
+              "Timed out waiting for the vhost to be deleted",
+              ReqData, Context);
+        {error, E} ->
+            Reason = iolist_to_binary(
+                       io_lib:format(
+                         "Error occurred while deleting vhost: ~tp",
+                         [E])),
+            rabbit_mgmt_util:internal_server_error(
+              Reason, ReqData, Context)
+    end.
 
 is_authorized(ReqData, Context) ->
     rabbit_mgmt_util:is_authorized_admin(ReqData, Context).