Prepare domains backend to allow retries

Current logic for domain deletion first checks that the domain exists, then removes the domain, and then runs the remove_domain hook. But this is a problem, because if any of the hooks fails, then retrying the domain removal won’t work, because it will be stopped at the first check for domain existence. Here we instead mark the domain as "in progress for deletion", which will already disable it, and only finally delete it _after_ all hook handlers have run hopefully successfully.
esl · Sep 26, 2022 · 3eea77f · 3eea77f
1 parent 6c3819f
commit 3eea77f
Show file tree

Hide file tree

Showing 12 changed files with 123 additions and 74 deletions.
diff --git a/big_tests/tests/graphql_domain_SUITE.erl b/big_tests/tests/graphql_domain_SUITE.erl
@@ -145,7 +145,7 @@ disable_domain(Config) ->
     ParsedResult = get_ok_value([data, domain, disableDomain], Result),
     ?assertMatch(#{<<"domain">> := ?EXAMPLE_DOMAIN, <<"enabled">> := false}, ParsedResult),
     {ok, Domain} = rpc(mim(), mongoose_domain_sql, select_domain, [?EXAMPLE_DOMAIN]),
-    ?assertEqual(#{host_type => ?HOST_TYPE, enabled => false}, Domain).
+    ?assertEqual(#{host_type => ?HOST_TYPE, status => disabled}, Domain).
 
 enable_domain(Config) ->
     Result = enable_domain(?EXAMPLE_DOMAIN, Config),

diff --git a/big_tests/tests/service_domain_db_SUITE.erl b/big_tests/tests/service_domain_db_SUITE.erl
@@ -370,7 +370,7 @@ db_get_all_dynamic(_) ->
 
 db_inserted_domain_is_in_db(_) ->
     ok = insert_domain(mim(), <<"example.db">>, <<"type1">>),
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
        select_domain(mim(), <<"example.db">>).
 
 db_inserted_domain_is_in_core(_) ->
@@ -387,7 +387,7 @@ db_deleted_domain_fails_with_wrong_host_type(_) ->
     ok = insert_domain(mim(), <<"example.db">>, <<"type1">>),
     {error, wrong_host_type} =
         delete_domain(mim(), <<"example.db">>, <<"type2">>),
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
         select_domain(mim(), <<"example.db">>).
 
 db_deleted_domain_from_core(_) ->
@@ -400,7 +400,7 @@ db_deleted_domain_from_core(_) ->
 db_disabled_domain_is_in_db(_) ->
     ok = insert_domain(mim(), <<"example.db">>, <<"type1">>),
     ok = disable_domain(mim(), <<"example.db">>),
-    {ok, #{host_type := <<"type1">>, enabled := false}} =
+    {ok, #{host_type := <<"type1">>, status := disabled}} =
        select_domain(mim(), <<"example.db">>).
 
 db_disabled_domain_not_in_core(_) ->
@@ -413,7 +413,7 @@ db_reenabled_domain_is_in_db(_) ->
     ok = insert_domain(mim(), <<"example.db">>, <<"type1">>),
     ok = disable_domain(mim(), <<"example.db">>),
     ok = enable_domain(mim(), <<"example.db">>),
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
        select_domain(mim(), <<"example.db">>).
 
 db_reenabled_domain_is_in_core(_) ->
@@ -528,7 +528,7 @@ db_records_are_restored_on_mim_restart(_) ->
     {error, not_found} = get_host_type(mim(), <<"example.com">>),
     service_enabled(mim()),
     %% DB still contains data
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
        select_domain(mim(), <<"example.com">>),
     %% Restored
     {ok, <<"type1">>} = get_host_type(mim(), <<"example.com">>).
@@ -542,9 +542,9 @@ db_record_is_ignored_if_domain_static(_) ->
     restart_domain_core(mim(), [{<<"example.com">>, <<"cfggroup">>}], [<<"dbgroup">>, <<"cfggroup">>]),
     service_enabled(mim()),
     %% DB still contains data
-    {ok, #{host_type := <<"dbgroup">>, enabled := true}} =
+    {ok, #{host_type := <<"dbgroup">>, status := enabled}} =
        select_domain(mim(), <<"example.com">>),
-    {ok, #{host_type := <<"dbgroup">>, enabled := true}} =
+    {ok, #{host_type := <<"dbgroup">>, status := enabled}} =
        select_domain(mim(), <<"example.net">>),
      %% Static DB records are ignored
     {ok, <<"cfggroup">>} = get_host_type(mim(), <<"example.com">>),
@@ -719,20 +719,20 @@ db_event_could_appear_with_lower_id(_Config) ->
 cli_can_insert_domain(Config) ->
     {"Added\n", 0} =
         mongooseimctl("insert_domain", [<<"example.db">>, <<"type1">>], Config),
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
         select_domain(mim(), <<"example.db">>).
 
 cli_can_disable_domain(Config) ->
     mongooseimctl("insert_domain", [<<"example.db">>, <<"type1">>], Config),
     mongooseimctl("disable_domain", [<<"example.db">>], Config),
-    {ok, #{host_type := <<"type1">>, enabled := false}} =
+    {ok, #{host_type := <<"type1">>, status := disabled}} =
         select_domain(mim(), <<"example.db">>).
 
 cli_can_enable_domain(Config) ->
     mongooseimctl("insert_domain", [<<"example.db">>, <<"type1">>], Config),
     mongooseimctl("disable_domain", [<<"example.db">>], Config),
     mongooseimctl("enable_domain", [<<"example.db">>], Config),
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
         select_domain(mim(), <<"example.db">>).
 
 cli_can_delete_domain(Config) ->
@@ -824,13 +824,13 @@ cli_disable_domain_fails_if_service_disabled(Config) ->
 rest_can_insert_domain(Config) ->
     {{<<"204">>, _}, _} =
         rest_put_domain(Config, <<"example.db">>, <<"type1">>),
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
         select_domain(mim(), <<"example.db">>).
 
 rest_can_disable_domain(Config) ->
     rest_put_domain(Config, <<"example.db">>, <<"type1">>),
     rest_patch_enabled(Config, <<"example.db">>, false),
-    {ok, #{host_type := <<"type1">>, enabled := false}} =
+    {ok, #{host_type := <<"type1">>, status := disabled}} =
         select_domain(mim(), <<"example.db">>).
 
 rest_can_delete_domain(Config) ->
@@ -941,13 +941,13 @@ rest_can_enable_domain(Config) ->
     rest_put_domain(Config, <<"example.db">>, <<"type1">>),
     rest_patch_enabled(Config, <<"example.db">>, false),
     rest_patch_enabled(Config, <<"example.db">>, true),
-    {ok, #{host_type := <<"type1">>, enabled := true}} =
+    {ok, #{host_type := <<"type1">>, status := enabled}} =
         select_domain(mim(), <<"example.db">>).
 
 rest_can_select_domain(Config) ->
     rest_put_domain(Config, <<"example.db">>, <<"type1">>),
     {{<<"200">>, <<"OK">>},
-     {[{<<"host_type">>, <<"type1">>}, {<<"enabled">>, true}]}} =
+     {[ {<<"status">>, <<"enabled">>}, {<<"host_type">>, <<"type1">>} ]}} =
         rest_select_domain(Config, <<"example.db">>).
 
 rest_cannot_select_domain_if_domain_not_found(Config) ->
@@ -1143,7 +1143,7 @@ erase_database(Node) ->
 
 prepare_test_queries(Node) ->
     case mongoose_helper:is_rdbms_enabled(domain()) of
-        true -> rpc(Node, mongoose_domain_sql, prepare_test_queries, [global]);
+        true -> rpc(Node, mongoose_domain_sql, prepare_test_queries, []);
         false -> ok
     end.
 

diff --git a/doc/migrations/5.1.0_6.0.0.md b/doc/migrations/5.1.0_6.0.0.md
@@ -15,3 +15,10 @@ For some endpoints, the response messages may be slightly different because of t
 ## CTL
 
 For some commands, the response messages may be slightly different because of the unification with other APIs.
+
+## Dynamic domains
+
+Removing a domain was a potentially troublesome operation: if the removal was to fail midway through the process, retrials wouldn't be accepted. This is fixed now, by first disabling and marking a domain for removal, then running all the handlers, and only on full success will the domain be removed. So if any failure is notified, the whole operation can be retried again.
+
+The database requires a migration, as the status of a domain takes now more than the two values a boolean allows, see the migrations for Postgres, MySQL and MSSQL in the [`priv/migrations`](https://github.com/esl/MongooseIM/tree/master/priv/migrations) directory.
+
diff --git a/priv/migrations/mssql_5.1.0_6.0.0.sql b/priv/migrations/mssql_5.1.0_6.0.0.sql
@@ -0,0 +1,2 @@
+-- DOMAINS
+sp_rename 'domains_settings.enabled', 'status', 'COLUMN';
diff --git a/priv/migrations/mysql_5.1.0_6.0.0.sql b/priv/migrations/mysql_5.1.0_6.0.0.sql
@@ -0,0 +1,4 @@
+-- DOMAINS
+ALTER TABLE domain_settings ALTER COLUMN enabled DROP DEFAULT;
+ALTER TABLE domain_settings CHANGE enabled status TINYINT NOT NULL;
+ALTER TABLE domain_settings ALTER COLUMN status SET DEFAULT 1;
diff --git a/priv/migrations/pgsql_5.1.0_6.0.0.sql b/priv/migrations/pgsql_5.1.0_6.0.0.sql
@@ -0,0 +1,10 @@
+-- DOMAINS
+ALTER TABLE domains_settings ALTER COLUMN enabled DROP DEFAULT;
+
+ALTER TABLE domains_settings
+  ALTER COLUMN enabled TYPE SMALLINT USING CASE WHEN enabled THEN 1 ELSE 0 END;
+
+ALTER TABLE domains_settings
+  RENAME enabled TO status;
+
+ALTER TABLE domains_settings ALTER COLUMN status SET DEFAULT 1;
diff --git a/priv/mssql2012.sql b/priv/mssql2012.sql
@@ -741,7 +741,7 @@ CREATE TABLE domain_settings (
     id BIGINT IDENTITY(1,1) PRIMARY KEY,
     domain VARCHAR(250) NOT NULL,
     host_type VARCHAR(250) NOT NULL,
-    enabled SMALLINT NOT NULL DEFAULT 1
+    status SMALLINT NOT NULL DEFAULT 1
 );
 
 -- A new record is inserted into domain_events, each time

diff --git a/priv/mysql.sql b/priv/mysql.sql
@@ -533,7 +533,7 @@ CREATE TABLE domain_settings (
     id BIGINT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
     domain VARCHAR(250) NOT NULL,
     host_type VARCHAR(250) NOT NULL,
-    enabled BOOLEAN NOT NULL DEFAULT true
+    status TINYINT NOT NULL DEFAULT 1
 );
 
 -- A new record is inserted into domain_events, each time

diff --git a/priv/pg.sql b/priv/pg.sql
@@ -490,7 +490,7 @@ CREATE TABLE domain_settings (
     id BIGSERIAL NOT NULL UNIQUE,
     domain VARCHAR(250) NOT NULL,
     host_type VARCHAR(250) NOT NULL,
-    enabled BOOLEAN NOT NULL DEFAULT true,
+    status SMALLINT NOT NULL DEFAULT 1,
     PRIMARY KEY(domain)
 );
 

diff --git a/src/domain/mongoose_domain_api.erl b/src/domain/mongoose_domain_api.erl
@@ -66,27 +66,39 @@ insert_domain(Domain, HostType) ->
             Other
     end.
 
+-type delete_domain_return() ::
+    ok | {error, static} | {error, unknown_host_type} | {error, service_disabled}
+    | {error, {db_error, term()}} | {error, wrong_host_type} | {error, {modules_failed, [module()]}}.
+
 %% Returns ok, if domain not found.
 %% Domain should be nameprepped using `jid:nameprep'.
--spec delete_domain(domain(), host_type()) ->
-    ok | {error, static} | {error, {db_error, term()}}
-    | {error, service_disabled} | {error, wrong_host_type} | {error, unknown_host_type}.
+-spec delete_domain(domain(), host_type()) -> delete_domain_return().
 delete_domain(Domain, HostType) ->
     case check_domain(Domain, HostType) of
         ok ->
-            Res = check_db(mongoose_domain_sql:delete_domain(Domain, HostType)),
-            case Res of
+            Res0 = check_db(mongoose_domain_sql:set_domain_for_deletion(Domain, HostType)),
+            case Res0 of
                 ok ->
                     delete_domain_password(Domain),
-                    mongoose_hooks:remove_domain(HostType, Domain);
-                _ ->
-                    ok
-            end,
-            Res;
+                    do_delete_domain_in_progress(Domain, HostType);
+                Other ->
+                    Other
+            end;
         Other ->
             Other
     end.
 
+%% This is ran only in the context of `do_delete_domain',
+%% so it can already skip some checks
+-spec do_delete_domain_in_progress(domain(), host_type()) -> delete_domain_return().
+do_delete_domain_in_progress(Domain, HostType) ->
+    case mongoose_hooks:remove_domain(HostType, Domain) of
+        #{failed := []} ->
+            check_db(mongoose_domain_sql:delete_domain(Domain, HostType));
+        #{failed := Failed} ->
+            {error, {modules_failed, Failed}}
+    end.
+
 -spec disable_domain(domain()) ->
     ok | {error, not_found} | {error, static} | {error, service_disabled}
     | {error, {db_error, term()}}.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		-- DOMAINS
		sp_rename 'domains_settings.enabled', 'status', 'COLUMN';