Merge pull request #3774 from esl/remove_domain/retries

Remove domain/retries

big_tests/tests/graphql_domain_SUITE.erl

@@ -103,7 +103,7 @@ create_domain(DomainName, Config) ->
     ParsedResult = get_ok_value([data, domain, addDomain], Result),
     ?assertEqual(#{<<"domain">> => DomainName,
         <<"hostType">> => ?HOST_TYPE,
-        <<"enabled">> => null}, ParsedResult).
+        <<"status">> => null}, ParsedResult).
 
 unknown_host_type_error_formatting(Config) ->
     DomainName = ?EXAMPLE_DOMAIN,
@@ -143,14 +143,14 @@ wrong_host_type_error_formatting(Config) ->
 disable_domain(Config) ->
     Result = disable_domain(?EXAMPLE_DOMAIN, Config),
     ParsedResult = get_ok_value([data, domain, disableDomain], Result),
-    ?assertMatch(#{<<"domain">> := ?EXAMPLE_DOMAIN, <<"enabled">> := false}, ParsedResult),
+    ?assertMatch(#{<<"domain">> := ?EXAMPLE_DOMAIN, <<"status">> := <<"DISABLED">>}, 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),
     ParsedResult = get_ok_value([data, domain, enableDomain], Result),
-    ?assertMatch(#{<<"domain">> := ?EXAMPLE_DOMAIN, <<"enabled">> := true}, ParsedResult).
+    ?assertMatch(#{<<"domain">> := ?EXAMPLE_DOMAIN, <<"status">> := <<"ENABLED">>}, ParsedResult).
 
 get_domains_by_host_type(Config) ->
     Result = get_domains_by_host_type(?HOST_TYPE, Config),
@@ -163,7 +163,7 @@ get_domain_details(Config) ->
     ParsedResult = get_ok_value([data, domain, domainDetails], Result),
     ?assertEqual(#{<<"domain">> => ?EXAMPLE_DOMAIN,
                    <<"hostType">> => ?HOST_TYPE,
-                   <<"enabled">> => true}, ParsedResult).
+                   <<"status">> => <<"ENABLED">>}, ParsedResult).
 
 delete_domain(Config) ->
     Result1 = remove_domain(?EXAMPLE_DOMAIN, ?HOST_TYPE, Config),
@@ -202,7 +202,7 @@ domain_admin_get_domain_details(Config) ->
     ParsedResult = get_ok_value([data, domain, domainDetails], Result),
     ?assertEqual(#{<<"domain">> => ?DOMAIN_ADMIN_EXAMPLE_DOMAIN,
                    <<"hostType">> => ?HOST_TYPE,
-                   <<"enabled">> => true}, ParsedResult).
+                   <<"status">> => <<"ENABLED">>}, ParsedResult).
 
 domain_admin_set_domain_password(Config) ->
     Result = set_domain_password(?DOMAIN_ADMIN_EXAMPLE_DOMAIN, <<"secret">>, Config),

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.
 

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.
+

priv/graphql/schemas/global/domain.gql

@@ -8,5 +8,14 @@ type Domain{
   "Domain hostType"
   hostType: String
   "Is domain enabled?"
-  enabled: Boolean
+  status: DomainStatus
+}
+
+enum DomainStatus {
+  "Domain is enabled and ready to route traffic"
+  ENABLED
+  "Domain is disabled and won't be loaded into MongooseIM"
+  DISABLED
+  "Domain has been marked for deletion and is disabled until all data is removed"
+  DELETING
 }

priv/migrations/mssql_5.1.0_6.0.0.sql

@@ -0,0 +1,2 @@
+-- DOMAINS
+sp_rename 'domains_settings.enabled', 'status', 'COLUMN';

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;

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;

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

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

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)
 );
 

src/domain/mongoose_domain_api.erl

@@ -34,10 +34,11 @@
 -ignore_xref([get_all_dynamic/0]).
 -ignore_xref([stop/0]).
 
+-type status() :: enabled | disabled | deleting.
 -type domain() :: jid:lserver().
 -type host_type() :: mongooseim:host_type().
 -type subdomain_pattern() :: mongoose_subdomain_utils:subdomain_pattern().
-
+-export_type([status/0]).
 
 -spec init() -> ok | {error, term()}.
 init() ->
@@ -66,27 +67,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()}}.